Skip to content

Conversation

@keshav-space
Copy link

@keshav-space keshav-space commented Aug 20, 2025

Adds test coverage using reference data provided by purl-spec https://github.com/aboutcode-org/purl-spec/tree/main/tests/types.
The test schema is available https://github.com/aboutcode-org/purl-spec/blob/main/schemas/purl-test.schema.json.

Note

Out of 481 reference tests, 85 are failing.

Related issue: aboutcode-org/purl-spec#30

@petergardfjall
Copy link
Contributor

Out of 481 reference tests, 85 are failing.

Hey, thanks for doing this. I started looking at the failing tests and hope to be able to put up a PR soon.
I did however run into, what I think are, problems with the test files (package-url/purl-spec#644, package-url/purl-spec#643).

But there is indeed a number of fixes needed to the code, not least is it not very good at encoding purls according to spec.

I based my code on your changes in this PR and have a few suggestions which I felt made life simpler when writing (and trying to fix broken) tests.

Would you mind me commenting although I'm no repo maintainer?

@keshav-space
Copy link
Author

Would you mind me commenting although I'm no repo maintainer?

Yes please, suggestions are welcome.

Copy link
Contributor

@petergardfjall petergardfjall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've proposed a few changes which made the tests a bit easier to work with for me at least. Feel free to incorporate them!

@petergardfjall
Copy link
Contributor

Hey, for you information I've opened a PR where I attempt to fix the code according to spec: #83

It still fails on a number of the new test cases but I've opened a few tickets since I don't understand the requirements that the test cases appear to place on library implementations:

Signed-off-by: Keshav Priyadarshi <[email protected]>
@keshav-space
Copy link
Author

@petergardfjall thanks, I've incorporated your suggestion into the PR.

@keshav-space
Copy link
Author

keshav-space commented Aug 28, 2025

It still fails on a number of the new test cases but I've opened a few tickets since I don't understand the requirements that the test cases appear to place on library implementations:

I might be wrong, but I think there was an in-flight PR by @TG1999 to fix the encoding related test in the purl-spec repo. I can't seem to find it.

@pombredanne
Copy link
Member

@petergardfjall do you want to get committer access?

@petergardfjall
Copy link
Contributor

@petergardfjall do you want to get committer access?

Hey @pombredanne! What does "committer access" mean in practice here?
Does it mean "maintainer". I can't say that I know all the ins and outs of the purl spec (although I try as you may have seen from my recent tickets about questions on the spec test case 😅 ).

I'm not sure I would be comfortable making commits without someone knowledgable to review them.

The purl spec is very important in the project I'm currently working on so quicker turnaround/responsiveness is desriable.
But not at the expense of correctness. 😂

@pombredanne
Copy link
Member

@petergardfjall this would give you commit rights, but we are not committing things without reviews for sure!
RFC posted at #84

@pombredanne
Copy link
Member

@shibumi @mcombuechen we need to adopt the new test suite here soon enough... any objection to this PR? Can review and approve? Next up will be to fix the tests.

@shibumi
Copy link
Collaborator

shibumi commented Oct 21, 2025

SGTM to me. We just have to fix the tests :)

@petergardfjall
Copy link
Contributor

I think this is good to go. Would another maintainer care to review?

With the code changes in #83 (using packageurl_test.go from this branch) and some needed updates to the purl-spec test suite I think the library is in a good (better) state.

Merging this PR first and then merging #83 (after review, of course) seems like a sensible course of action to me.

Copy link
Contributor

@petergardfjall petergardfjall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving it my approval in case that helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants