Adds more test cases for npm #638
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -254,6 +254,316 @@ | |
| "expected_output": "pkg:npm/[email protected]#googleapis/api/annotations", | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Negative Test: An npm purl with an un-encoded @ in scope should fail parsing", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/@angular/[email protected]", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse due to unencoded '@' in the namespace" | ||
| }, | ||
| { | ||
| "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" | ||
| }, | ||
|
Comment on lines
+267
to
+275
Contributor
There was a problem hiding this comment. This is a valid PURL and must parse correctly as name "React" with an uppercase R.
Contributor
Author
There was a problem hiding this comment. 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
Contributor
There was a problem hiding this comment. This has been a known issue with 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 | ||
| }, | ||
|
Comment on lines
+276
to
+284
Contributor
There was a problem hiding this comment. This test is wrong, and also conflicts with the previous test. The correct output is
Contributor
Author
There was a problem hiding this comment. Agreed. This normalization test must be rewritten in a different manner. |
||
| { | ||
| "description": "Positive Test: An npm purl with no version is valid", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/react", | ||
| "expected_output": { | ||
| "type": "npm", | ||
| "namespace": null, | ||
| "name": "react", | ||
| "version": null, | ||
| "qualifiers": null, | ||
| "subpath": null | ||
| }, | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Positive Test: A scoped npm purl with no version is valid", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/%40angular/core", | ||
| "expected_output": { | ||
| "type": "npm", | ||
| "namespace": "@angular", | ||
| "name": "core", | ||
| "version": null, | ||
| "qualifiers": null, | ||
| "subpath": null | ||
| }, | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Negative Test: An npm purl with an empty version should fail", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/foobar@", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse due to an empty version string" | ||
| }, | ||
| { | ||
| "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" | ||
| }, | ||
|
Comment on lines
+326
to
+343
Contributor
There was a problem hiding this comment. Why should these fail? PURL does not (and IMO should not) define any rules to forbid this.
Contributor
Author
There was a problem hiding this comment. Good question. My bot includes information from additional sources. https://github.com/npm/validate-npm-package-name/blob/main/test/index.js#L71
Contributor
There was a problem hiding this comment. 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 |
||
| { | ||
| "description": "Edge Case Test: Version with pre-release and build metadata should be preserved", | ||
| "test_group": "advanced", | ||
| "test_type": "roundtrip", | ||
| "input": "pkg:npm/[email protected]+build.456", | ||
| "expected_output": "pkg:npm/[email protected]+build.456", | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Positive Test: Package name with dots and hyphens is valid", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]", | ||
| "expected_output": { | ||
| "type": "npm", | ||
| "namespace": null, | ||
| "name": "my-package.js", | ||
| "version": "1.0.0", | ||
| "qualifiers": null, | ||
| "subpath": null | ||
| }, | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "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 | ||
| }, | ||
|
Comment on lines
+369
to
+377
Contributor
There was a problem hiding this comment. This test makes the previous (incorrect) test about lowercasing the name redundant. |
||
| { | ||
| "description": "Negative Test: A purl with only a type should fail parsing", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse because name is a required component" | ||
| }, | ||
| { | ||
| "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" | ||
| }, | ||
|
Comment on lines
+387
to
+404
Contributor
There was a problem hiding this comment. Again, PURL does not define validation rules to forbid this so the tests should not test for that validation.
Contributor
Author
There was a problem hiding this comment. 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": "Positive Test: An npm purl with a subpath containing unicode characters (emoji) should roundtrip correctly", | ||
| "test_group": "advanced", | ||
| "test_type": "roundtrip", | ||
| "input": "pkg:npm/[email protected]#src/%F0%9F%98%83/file.js", | ||
| "expected_output": "pkg:npm/[email protected]#src/%F0%9F%98%83/file.js", | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "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 | ||
| }, | ||
|
Comment on lines
+414
to
+456
Contributor
There was a problem hiding this comment. Do these tests have anything to do with NPM? They seem like they should be basic tests for the core spec.
Contributor
Author
There was a problem hiding this comment. I noticed that there were no such tests. So, I pushed a commit to showcase some. |
||
| { | ||
| "description": "Edge Case Test: An npm purl with a unicode character in the version string should roundtrip", | ||
| "test_group": "advanced", | ||
| "test_type": "roundtrip", | ||
| "input": "pkg:npm/[email protected]%F0%9F%98%83", | ||
| "expected_output": "pkg:npm/[email protected]%F0%9F%98%83", | ||
| "expected_failure": false, | ||
| "expected_failure_reason": "Version is treated as verbatim, even if it's not valid SemVer" | ||
| }, | ||
| { | ||
| "description": "Positive test for a package with a name containing a problematic but valid character like a dot", | ||
| "test_group": "advanced", | ||
| "test_type": "roundtrip", | ||
| "input": "pkg:npm/[email protected]", | ||
| "expected_output": "pkg:npm/[email protected]", | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Negative Test: An npm purl with a Chinese character in the name should fail parsing", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/%E8%BD%AF%E4%BB%B6%E5%8C%[email protected]", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse because the name contains invalid characters" | ||
| }, | ||
| { | ||
| "description": "Negative Test: An npm purl with a Japanese character in the scope should fail parsing", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/%40%E3%83%91%E3%83%83%E3%82%B1%E3%83%BC%E3%82%B8/[email protected]", | ||
| "expected_output": null, | ||
| "expected_failure": true, | ||
| "expected_failure_reason": "Should fail to parse because the scope contains invalid characters" | ||
| }, | ||
| { | ||
| "description": "Positive Test: A subpath with Chinese characters should be parsed correctly", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]#dist/%E6%96%87%E4%BB%B6/main.js", | ||
| "expected_output": { | ||
| "type": "npm", | ||
| "namespace": null, | ||
| "name": "some-package", | ||
| "version": "1.2.3", | ||
| "qualifiers": null, | ||
| "subpath": "dist/文件/main.js" | ||
| }, | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Positive Test: A subpath with Japanese characters should roundtrip correctly", | ||
| "test_group": "advanced", | ||
| "test_type": "roundtrip", | ||
| "input": "pkg:npm/[email protected]#src/%E3%83%A9%E3%82%A4%E3%83%96%E3%83%A9%E3%83%AA/index.js", | ||
| "expected_output": "pkg:npm/[email protected]#src/%E3%83%A9%E3%82%A4%E3%83%96%E3%83%A9%E3%83%AA/index.js", | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Positive Test: A subpath with mixed Chinese and Japanese characters should parse correctly", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]#src/%E6%96%87%E4%BB%B6/%E3%83%95%E3%82%A9%E3%83%AB%E3%83%80/script.js", | ||
| "expected_output": { | ||
| "type": "npm", | ||
| "namespace": null, | ||
| "name": "some-package", | ||
| "version": "1.2.3", | ||
| "qualifiers": null, | ||
| "subpath": "src/文件/フォルダ/script.js" | ||
| }, | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| }, | ||
| { | ||
| "description": "Positive Test: Version string with Chinese characters should parse correctly", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]%E7%89%88%E6%9C%AC.1", | ||
| "expected_output": { | ||
| "type": "npm", | ||
| "namespace": null, | ||
| "name": "mypackage", | ||
| "version": "1.0.0-版本.1", | ||
| "qualifiers": null, | ||
| "subpath": null | ||
| }, | ||
| "expected_failure": false, | ||
| "expected_failure_reason": "Version is treated as verbatim" | ||
| }, | ||
| { | ||
| "description": "Positive Test: Qualifier value with Japanese characters should be parsed correctly", | ||
| "test_group": "advanced", | ||
| "test_type": "parse", | ||
| "input": "pkg:npm/[email protected]?vcs_url=https://github.com/user/%E3%83%AA%E3%83%9D%E3%82%B8%E3%83%88%E3%83%AA.git", | ||
| "expected_output": { | ||
| "type": "npm", | ||
| "namespace": null, | ||
| "name": "some-package", | ||
| "version": "1.2.3", | ||
| "qualifiers": { | ||
| "vcs_url": "https://github.com/user/リポジトリ.git" | ||
| }, | ||
| "subpath": null | ||
| }, | ||
| "expected_failure": false, | ||
| "expected_failure_reason": null | ||
| } | ||
| ] | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
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:
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.