Skip to content

feat: introduce apitester tool#4266

Merged
another-rex merged 21 commits into
google:masterfrom
ackama:apitester
Nov 2, 2025
Merged

feat: introduce apitester tool#4266
another-rex merged 21 commits into
google:masterfrom
ackama:apitester

Conversation

@G-Rath

@G-Rath G-Rath commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator

Hopefully the readme explains most of this, but at a high-level this introduces a new tool based off osv-scanner to make it easy to test API changes.

Tests are provided as "cassettes" which we use go-vcr to load since that has the bonus benefit of being compatible with cassettes we'll be using in osv-scanner, though we do format the request.body to be multi-line so it's easier to understand what the query under test is.

@G-Rath

G-Rath commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator Author

@another-rex I think this is pretty much ready to go, just want to check if we should do more or less JSON replacing

another-rex
another-rex previously approved these changes Oct 29, 2025

@another-rex another-rex left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great! I'm on the fence about whether to compress the versions list, as it is really really long, but we probably do want to test its output. Probably the best compromise is to just to replace versions array with the length of the list.

@michaelkedar Can you take a look as well?

@G-Rath

G-Rath commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator Author

yeah I was thinking about that and then realized in osv-scanner we actually replace vulns with just their IDs, so I figured I'd get this up so others could vote.

Reducing them to just a count makes sense I think, especially if we do a deduplicate too

@michaelkedar

Copy link
Copy Markdown
Member

This looks good to me!

I wonder if it'd be a good idea to replace some of the stripped fields with a base64 blob hash or or something, so we can see if they change? (Though, I know if changes in database_specific are really actionable for us).

@G-Rath

G-Rath commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator Author

so we can see if they change? (Though, I know if changes in database_specific are really actionable for us).

fwiw just as I realized I hadn't mentioned this here: the reason I'm placeholder'ing database_specific is because the field order is not stable e.g. if you look at https://api.osv.dev/v1/vulns/GHSA-cpwx-vrp4-4pq7 and refresh the page a bunch, you'll see the fields jump around

@another-rex

Copy link
Copy Markdown
Contributor

We probably should do a sort keys on database specific before we return it

@michaelkedar

Copy link
Copy Markdown
Member

the reason I'm placeholder'ing database_specific is because the field order is not stable

This would also be true for ecosystem_specific e.g. in https://api.osv.dev/v1/vulns/ASB-A-212610736

We probably should do a sort keys on database specific before we return it

From the API side, this seems to be something happening in the ESPv2 translation, and I don't think we have control over it.
We could be sorting it in the tests. We probably should be sorting the whole API response - I don't think we can generally guarantee any particular ordering of JSON keys.

@G-Rath G-Rath marked this pull request as ready for review October 30, 2025 18:58
@G-Rath

G-Rath commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator Author

I think unless we think that not having database_specific in the snapshots is an issue, let's stick with what we have for now rather than having the keys sorted within the tests as I think that could hide some interesting data (at worst knowing what fields are unstable, at best it could highlight an unexpected side effect in a particular change such as a field being touched that shouldn't be)

@another-rex

Copy link
Copy Markdown
Contributor

/gcbrun

@another-rex

Copy link
Copy Markdown
Contributor

hmm the force push might have broke the CI somehow...

@another-rex another-rex merged commit ea947f0 into google:master Nov 2, 2025
16 of 18 checks passed
@G-Rath G-Rath deleted the apitester branch November 4, 2025 17:58
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.

3 participants