-
Notifications
You must be signed in to change notification settings - Fork 186
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
test(ecosystems): add tests for ecosystem consistency #2615
base: master
Are you sure you want to change the base?
test(ecosystems): add tests for ecosystem consistency #2615
Conversation
Test that ecosystem name hardcoding is internally consistent and consistent with the OSV Schema
osv/ecosystems/ecosystems_test.py
Outdated
for ecosystem in self.canonical_ecosystems: | ||
self.assertTrue( | ||
self.schema_ecosystems.match(ecosystem), | ||
msg=f'"{ecosystem}" not defined in schema') |
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.
Wondering if there might be a way to also check the inverse of this.
i.e. check if there are any ecosystems that are allowed in the schema that ecosystems.get()
doesn't have.
Though, I can't really think of a way to do this given it's a regex pattern (unless we want to fuzz all possible strings)
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.
Wondering if there might be a way to also check the inverse of this.
i.e. check if there's any ecosystems that are allowed in the schema thatecosystems.get()
doesn't have.
I had thought of that, but then it's somewhat to be expected as new ecosystems onboard, that they get defined in the schema first and then in OSV.dev code, so we wouldn't want this test to start failing when that happens, right?
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 suppose that makes sense.
On the other hand, the tests failing would be good to signal that we need to add the ecosystem before we start attempting to ingest them into the database.
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.
On the other hand, the tests failing would be good to signal that we need to add the ecosystem before we start attempting to ingest them into the database.
That makes me think there might be an opportunity to flag a premature addition to sources{_test}.yaml 🤔
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.
No, that isn't going to work because there's no direct linkage between a source and the ecosystems it provides...
Address reviewer nit
…lock/osv.dev into test_ecosystems_against_schema
Test that ecosystem name hardcoding is internally consistent and consistent with the OSV Schema
Requires OSV Schema submodule to be at or past ossf/osv-schema@84969b4 (currently unreleased) to pass.