-
Notifications
You must be signed in to change notification settings - Fork 191
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
Speed up upstream tests #490
Conversation
Internally, the difference between both has faded and updated_advisories is preferred. Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Earlier, one batch of advisories was requested from updated_advisories method of the respective importers. This was inefficient as not all importers respect batching internally. Eventually, we wish to eliminate batches as well ( # 338 ). Now, the updated_advisories method of each importer is expected to create at least one Advisory object. If it does so, the importer is marked working. This brings major performance improvement. It is a necessity to improve this test as GitHub only allows 6 hrs of workflow time. Before: ~6hrs, now ~9 minutes Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
899101c
to
6b29388
Compare
About the comment in PR :
No, it fetches everything https://github.com/nexB/vulnerablecode/blob/d94f4f6aefa24ad1e3cce869a59da92a8c6abb75/vulnerabilities/tests/test_upstream.py#L17 . If you're talking about https://github.com/nexB/vulnerablecode/blob/d94f4f6aefa24ad1e3cce869a59da92a8c6abb75/vulnerabilities/tests/test_upstream.py#L15 , then the About the PR
a. These tests are for checking whether our code is able to parse and process upstream data. The upstream data can change any time, so we always need check everything. Checking/Processing just one advisory IMHO defeats the whole purpose. b. The changed tests are super cryptic (I doubt I still understand it). |
Is there any rationale behind using batches of size 1 here in that case ?
I seem to have misinterpreted the batch usage here. I will fix the commit message once other problems are resolved. Regardless, as we are moving towards non batched processing, the usage of batch should be minimized (if not eliminated) in future code.
The earlier implementation only made sure that the
A bird eye view would be that the |
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 hate the monkey patching but this makes full sense in the current state of the codebase and the lack of consistency on the importers side. Therefore this is a go IMHO.
Fixes: #437
Earlier, one batch of advisories was requested from updated_advisories
method of the respective importers. This was inefficient as not all
importers respect batching internally. Eventually, we wish to eliminate
batches as well ( #338 ).
Now, the updated_advisories method of each importer is expected to
create at least one Advisory object. If it does so, the importer is
marked working.
This brings major performance improvement. It is a necessity to improve
this test as GitHub only allows 6 hrs of workflow time.
Before: ~6hrs, now ~9 minutes