-
Notifications
You must be signed in to change notification settings - Fork 206
Adds more test cases for npm #638
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?
Conversation
Signed-off-by: Prabhu Subramanian <[email protected]>
Signed-off-by: Prabhu Subramanian <[email protected]>
| "test_type": "parse", | ||
| "input": "pkg:npm/@angular/[email protected]", | ||
| "expected_output": null, | ||
| "expected_failure": true, |
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.
Is this true? Last I heard it was expected to parse because there was no instruction to specifically reject unencoded @ characters in positions where they could require encoding depending on the rest of the PURL.
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.
purl-spec/types/npm-definition.json
Line 16 in 96d6f8c
| "note": "The namespace is used for the scope of a scoped NPM package. The npm scope @ sign prefix is always percent encoded, as it was in the early days of npm scope." |
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.
That's what the note says, but:
- I don't think it is possible for a type spec like NPM to alter the percent encoding behavior. Percent encoding is not type-specific.
- Whether the @ sign is supposed to be encoded or not does not specify the behavior of parsing when it is not.
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.
This is yet another example of the difference between the specification and the instructions for parser developers.
According to the proposed parsing algorithm, @ can be safely used as the test input. Of course the canonical form will need to encode the character as %40.
| { | ||
| "description": "Negative Test: An npm purl with an uppercase name should fail parsing", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse due to uppercase characters in the name" | ||
| }, |
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.
This is a valid PURL and must parse correctly as name "React" with an uppercase R.
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.
These are tests based only on the contents in the type definitions. The PRs are drafted to encourage comments like these, so thanks a lot.
To me (and my bot), the test looks valid because there is a note that explicitly says the name must be lowercased.
purl-spec/types/npm-definition.json
Line 21 in 96d6f8c
| "note": "Per the package.json spec, new package 'must not have uppercase letters in the name', therefore the name must be lowercased. The npm name used to be case sensitive in the early days for some old packages." |
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.
This has been a known issue with pkg:npm since 2021: #136.
Even following the incorrect spec, if the name is case insensitive, then parsing a name with mixed case should not result in an error.
| { | ||
| "description": "Normalization Test: An npm purl with an uppercase name should be lowercased on roundtrip", | ||
| "test_group": "advanced", | ||
| "test_type": "roundtrip", | ||
| "input": "pkg:npm/[email protected]", | ||
| "expected_output": "pkg:npm/[email protected]", | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, |
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.
This test is wrong, and also conflicts with the previous test. The correct output is pkg:npm/LoDash.
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. This normalization test must be rewritten in a different manner.
| { | ||
| "description": "Negative Test: An npm purl with a name starting with a dot should fail", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse because the name starts with a dot" | ||
| }, | ||
| { | ||
| "description": "Negative Test: An npm purl with a name starting with an underscore should fail", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse because the name starts with an underscore" | ||
| }, |
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.
Why should these fail? PURL does not (and IMO should not) define any rules to forbid this.
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.
Good question. My bot includes information from additional sources.
https://github.com/npm/validate-npm-package-name/blob/main/test/index.js#L71
https://github.com/npm/validate-npm-package-name/blob/main/test/index.js#L91
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.
Including name validation rules makes PURL implementations more complicated to write and more likely to be wrong, and PURL more likely to break as ecosystems evolve. I don't think this adds any benefit because whether somebody writes pkg:npm/_foo (invalid according to this test) or pkg:npm/a0103e9a-398d-4136-bced-c1cdbe95ebcd, neither PURL can be resolved to a package today.
| { | ||
| "description": "Negative Test: An npm purl with a unicode character (emoji) in the name should fail parsing", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/foo%F0%9F%98%[email protected]", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse because the name contains characters not allowed by npm naming rules" | ||
| }, | ||
| { | ||
| "description": "Negative Test: An npm purl with a unicode character (emoji) in the scope should fail parsing", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/%40foo%F0%9F%98%83/[email protected]", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse because the scope contains characters not allowed by npm naming rules" | ||
| }, |
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.
Again, PURL does not define validation rules to forbid this so the tests should not test for that validation.
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.
npm does forbid, though. I agree that these shouldn't be in the purl-spec tests but must be present in SBOM tools at least.
| { | ||
| "description": "Normalization Test: Both scope and name with uppercase letters are lowercased", | ||
| "test_group": "advanced", | ||
| "test_type": "roundtrip", | ||
| "input": "pkg:npm/%40MyScope/[email protected]", | ||
| "expected_output": "pkg:npm/%40myscope/[email protected]", | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, |
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.
This test makes the previous (incorrect) test about lowercasing the name redundant.
| { | ||
| "description": "Positive Test: An npm purl with a subpath containing unicode characters (emoji) should parse correctly", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]#src/%F0%9F%98%83/file.js", | ||
| "expected_output": { | ||
| "type": "npm", | ||
| "namespace": null, | ||
| "name": "foo", | ||
| "version": "1.0.0", | ||
| "qualifiers": null, | ||
| "subpath": "src/😃/file.js" | ||
| }, | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Positive Test: An npm purl with a qualifier value containing unicode characters (emoji) should roundtrip correctly", | ||
| "test_group": "advanced", | ||
| "test_type": "roundtrip", | ||
| "input": "pkg:npm/[email protected]?vcs_url=https://a.com/b/c%F0%9F%98%83d.git", | ||
| "expected_output": "pkg:npm/[email protected]?vcs_url=https://a.com/b/c%F0%9F%98%83d.git", | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Positive Test: An npm purl with a qualifier value containing unicode characters (emoji) should parse correctly", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]?vcs_url=https://a.com/b/c%F0%9F%98%83d.git", | ||
| "expected_output": { | ||
| "type": "npm", | ||
| "namespace": null, | ||
| "name": "foo", | ||
| "version": "1.0.0", | ||
| "qualifiers": { | ||
| "vcs_url": "https://a.com/b/c😃d.git" | ||
| }, | ||
| "subpath": null | ||
| }, | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, |
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.
Do these tests have anything to do with NPM? They seem like they should be basic tests for the core spec.
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.
I noticed that there were no such tests. So, I pushed a commit to showcase some.
These additional tests were vibe-coded using cdx1-pro trained with the new enhanced purl schema definitions. The tests were manually reviewed, enhanced, and re-tested with the
make checkcommand.Methodology
Technically, any frontier model or LLMs tuned with the new purl4ml repo can be used to generate decent-quality additional tests for PURL types.
Prompt
Screenshots