-
Notifications
You must be signed in to change notification settings - Fork 206
discussion-draft for tests.md #747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,127 @@ | ||
| ## Tests | ||
|
|
||
| To support the language-neutral testing of `purl` implementations, a test | ||
| suite is provided as JSON document named `test-suite-data.json`. This JSON | ||
| document contains an array of objects. Each object represents a test with | ||
| these key/value pairs some of which may not be normalized: | ||
|
|
||
| - **purl**: a `purl` string. | ||
| - **canonical**: the same `purl` string in canonical, normalized form | ||
| - **type**: the `type` corresponding to this `purl`. | ||
| - **namespace**: the `namespace` corresponding to this `purl`. | ||
| - **name**: the `name` corresponding to this `purl`. | ||
| - **version**: the `version` corresponding to this `purl`. | ||
| - **qualifiers**: the `qualifiers` corresponding to this `purl` as an object | ||
| of {key: value} qualifier pairs. | ||
| - **subpath**: the `subpath` corresponding to this `purl`. | ||
| - **is_invalid**: a boolean flag set to true if the test should report an | ||
| error | ||
|
|
||
| To test `purl` parsing and building, a tool can use this test suite and for | ||
| every listed test object, run these tests: | ||
|
|
||
| - parsing the test canonical `purl` then re-building a `purl` from these | ||
| parsed components should return the test canonical `purl` | ||
|
|
||
| - parsing the test `purl` should return the components parsed from the test | ||
| canonical `purl` | ||
|
|
||
| - parsing the test `purl` then re-building a `purl` from these parsed | ||
| components should return the test canonical `purl` | ||
|
|
||
| - building a `purl` from the test components should return the test canonical | ||
| `purl` | ||
| The Package-URL (PURL) specification provides a JSON Schema and test | ||
| files to support language-neutral testing of PURL implementations. The | ||
| objectives for the PURL schema and test files are to enable tools to conform | ||
| to the PURL specification for tool functions such as: | ||
| - build a canonical PURL string from a set of PURL component-level data | ||
| - parse a canonical PURL string into a set of PURL components | ||
| - validate an input PURL string and optionally provide warning or info | ||
| messages or correct errors in the input PURL string | ||
|
|
||
| The current JSON schema is available at: `purl-spec/schemas/purl-test.schema-0.1.json`. | ||
|
|
||
| The test files are available at: | ||
| - `purl-spec/tests/spec/`: This folder contains JSON test files that are not | ||
| for a specific PURL type. | ||
| - `specification-test.json` - This file contains an array of test objects | ||
| that primarily cover testing the validity of individual PURL components or | ||
| separators between a pair of PURL components. | ||
| - component-`test.json`: These are test files for a specific PURL component. | ||
| See https://github.com/package-url/purl-spec/pull/738 which adds | ||
| `tests/spec/qualifiers.json`. | ||
| - `purl-spec/tests/types/`: This folder contains one JSON test file for each | ||
| registered PURL type. These tests should be focused on test cases that are | ||
| specific to a PURL type, such as those for namespace or qualifiers. | ||
|
|
||
| Two key properties in the PURL test JSON schema are: | ||
|
|
||
| **Test groups** | ||
|
|
||
| There are two PURL test groups: | ||
| - **base**: Test group for base conformance tests. Base tests are pass/fail. | ||
| - **advanced**: Test group for advanced tests. Advanced tests are more | ||
| permissive than base tests. They may provide severity messages or correct | ||
| errors. | ||
|
|
||
| *Discussion point:* | ||
| - *Is there a better name than advanced for the second test | ||
| group? perhaps Permissive? Or name the groups Strict and Permissive?]* | ||
|
|
||
| **Test types** | ||
|
|
||
| There are four PURL test types: | ||
| - **build**: A test to build a canonical PURL output string from an input of | ||
| decoded PURL components. See also `/docs/how-build.md`. | ||
| - **parse**: A test to parse decoded components from a canonical PURL | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests expect something that AFAIK is not specified anywhere. Implementations are expected to canonicalize during parsing and during building. If an implementation does not do this automatically (at least one implementation intentionally doesn't) then the test runner needs to do that or it will fail the tests. It's kind of obvious once you start looking at the test failures, but it would probably be good to specify that the implementation is expected to do that.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added: |
||
| input string. See also `/docs/how-parse.md`. | ||
| - **roundtrip**: A test to parse an input PURL string and then rebuild it as a | ||
| canonical PURL output string. | ||
| - **validation**: A test to validate a PURL input string and report severity | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the difference between a validation test and a parsing test? They both parse a PURL and they both check that the expected values are produced.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The output from a parsing test is the set of decoded PURL components.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The output from a building test is also a PURL string. PURL implementations generally have two operations: they can turn a PURL string into its components, and they can turn components into PURL strings. I can't think of something that round trip or validation tests could do that parsing or building tests can't already do. There's already some discussion about getting rid of round trip tests, and I wonder if this is another thing that will later be getting removed.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matt-phylum What do you suggest we call the proposed validation test type? Do you recommend 2 sub-types for a parse test where one returns the components and another returns a string?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why there would be a parse test that returns a string. If the parse is correct, the components will be correct. Once the input has been parsed, there is no memory of what it was before being parsed, so a test that parsing
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The apparent difference is in the expected test output. For the current parse test type the expected output is a set of decoded PURL components. For the proposed validation test type the expected output is a PURL string. I can see how you can get the string output by combining a parse test and a build test - are you recommending that we should not have that combination as a test type and leave the combination processing to the tool/implementation?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless there is an intention that these tests are only used by implementations that have a dedicated PURL string -> canonicalized PURL string routine that isn't implemented by parsing and building, I don't think it makes sense to have a test that parse+build for a given input string produces a specific output string. It's easier for implementers to test that both parsing and building work as expected and it avoids bugs where canonicalization is applied differently when parsing or building.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matt-phylum Thank you. Your perspective is clear to me now. |
||
| messages - info, warning or error. A validation test may optionally correct | ||
| errors in an output PURL string. | ||
|
|
||
| *Discussion points:* | ||
| - *The validation test type is currently a proposed PURL test schema change. | ||
| See https://github.com/package-url/purl-spec/pull/614.* | ||
| - *Do we need both roundtrip and validation test types?* | ||
| - *Should the proposed validation test type be split into two test types for:* | ||
| - *validation: message output* | ||
| - *remediation: corrects errors and also provides messages documenting the | ||
| corrections* | ||
| - *For tests in the advanced test group we may want to distinguish between | ||
| validation test types that return only a validation message and a remediation | ||
| test type which corrects an error or errors and also returns a message (see | ||
| below) that explains the remediation.* | ||
| - *For example, the first test in `tests/types/pypi.test.json` is:*\ | ||
| "description": "pypi names have special rules and not case sensitive. | ||
| Roundtrip an input purl to canonical.",\ | ||
| "test_group": "advanced",\ | ||
| "test_type": "roundtrip",\ | ||
| "input": "pkg:PYPI/Django_[email protected]1",\ | ||
| "expected_output": "pkg:pypi/django-[email protected]1",\ | ||
| "expected_failure": false,\ | ||
| "expected_failure_reason": null | ||
|
|
||
| *This test case is arguably an example of an advanced remediation test case | ||
| because it corrects two errors. An advanced validation test case should | ||
| arguably return two error messages such as:* | ||
| - *The pkg component must be lowercased.* | ||
| - *The name component must be lowercased.* | ||
|
|
||
| *It would be helpful for the remediation test case to also provide similar | ||
| warning messages documenting the remediation steps.* | ||
|
|
||
| **Test case basics** | ||
|
|
||
| The standard error-handling behaviour for all test cases is based on two | ||
| properties from the PURL test schema: | ||
| - `expected_failure` | ||
| - description: "true if this test input is expected to fail to be | ||
| processed." | ||
| - type: boolean | ||
| - default: false | ||
|
|
||
| - `expected_failure_reason`: "The reason why this test is is expected to fail | ||
| if expected_failure is true." | ||
| - default: null | ||
|
|
||
| In the case of a test case failure the `expected_output` is null. | ||
|
|
||
| *Discussion points:* | ||
| - *The current test schema does not include any specific properties for error | ||
| or other test result messages. Most of the test cases with an expected | ||
| failure are in the file `test/spec/specification-test.json`. Based on those | ||
| examples a tool could usually derive an error message from the description, | ||
| but the message would be indirect. For example from the first test case the | ||
| description is: "pypi names have special rules and not case sensitive". A | ||
| better message could be:"The name component for a PyPI package is not case | ||
| senstive".* | ||
|
Comment on lines
+108
to
+109
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are special rules. PURL actually has the wrong rules (#262), but the most important part of the provided example test is checking that the underscore is converted into a hyphen.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to: "The name component for a PyPI package is not case sensitive and requires changing a consecutive dash, underscore or dot character to a single dash". |
||
| - *The PR to add a validation test type (https://github.com/package-url/purl-spec/pull/614) proposes a new `purl validation message` property with three levels of | ||
| validation severity messages:* | ||
| - *info: "Informational validation message"* | ||
| - *warning: "Warning validation message* | ||
| - *error: "Error validation message"* | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is necessary or beneficial for PURL to specify error message strings to be produced by implementations. It seems potentially reasonable to have an explanation of why the test fails, but specifying the errors produced in this way prohibits or discourages implementations from handling errors in better or more customary ways. Implementations languages that throw exceptions or return typed results/outcomes should return typed errors, ie a syntactically invalid PURL and a PURL that fails package-type-specific validation should result in different types or enum values such that the consuming software doesn't need to analyze a string to determine what's going on. It's possible to specify that there is a conversion from however the implementation specifies errors into a string, but this seems more useful for the purposes of this test case than in actual usage.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea of adding these messages was introduced with #614 which is pending the conclusion of this discussion.
|
||
|
|
||
| *We will need to clearly define the difference between an info message and | ||
| a warning message.* | ||
|
Comment on lines
+116
to
+117
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do any PURL implementations support info or warning messages? Do any consumers check those messages? Are there meaningful messages to generate? Implementations would need to either store these messages on the parsed PURL or return a wrapper that includes these messages.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This discussion section is not based on what current PURL implementations do. The proposed set of messages are from #614 which is on hold pending the conclusion of this discussion. |
||
|
|
||
| *NB - there is no message proposed for a successful test.* | ||
|
|
||
| - *We should add message levels for all test types, not just the validation | ||
| test type.* | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parse tests historically do not need their inputs to be canonical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed and not sure why they should be.
Updated description of parse test type to: "A test to parse decoded components from a PURL input string."
from: "A test to parse decoded components from a canonical PURL input string."