Skip to content
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: Fix tests to reduce chance of 'database is locked' error. #4651 #4677

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

giacob500
Copy link
Contributor

This pull request addresses and improves the stability of the tests related to database access, specifically targeting the database is locked error in SQLite. The error typically occurs when database operations are attempted simultaneously, leading to contention and test failures.

Key Changes:

  • Retry Logic for SQLite Queries: Added a helper function execute_with_retry to gracefully handle SQLite database lock errors. This function retries the query a few times with a short delay before failing, reducing the chance of encountering lock issues.
  • Improved Test Handling: In tests that interact with the database (e.g., test_import_export_json), calls to the database now utilize the retry logic to ensure smooth execution even in the event of temporary locking.
  • Synchronous Grouping: Updated test marking for better clarity and organization. Tests that require external systems or involve long operations have been moved into the synchronous group to ensure that they run without asynchronous-related issues.

These improvements should help reduce flakiness in the test suite, particularly for long-running tests interacting with the SQLite database.

Related Issue:

@giacob500
Copy link
Contributor Author

giacob500 commented Jan 6, 2025

I decided to create my own marker @pytest.mark.synchronous to make the role of synchronous functions explicit, however pytest should not require it. In theory, by removing line 30, 40 of test/test_cvedb.py and the updates made to setup.cfg we should be able to obtain the same result.

Let me know your considerations, as I don't want to add bloat code. Thanks.

Copy link
Contributor

@terriko terriko 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 set the tests to run, but... I think if we're going to do a retry, we should be doing it in cve-bin-tool itself, not only the test suite. Could you look into moving your code there?

@giacob500
Copy link
Contributor Author

Hi Terri, when you say

Could you look into moving your code there?

do you mean removing the retry logic from the tests entirely and implementing it solely in the core cve-bin-tool code? Or should it remain in both the tests and the core? Just want to ensure I follow your intended approach. Thanks!

@terriko
Copy link
Contributor

terriko commented Jan 14, 2025

do you mean removing the retry logic from the tests entirely and implementing it solely in the core cve-bin-tool code? Or should it remain in both the tests and the core? Just want to ensure I follow your intended approach. Thanks!

The former: there should be no retry at all in the tests because the retry will be handled in the code (probably in cvedb.py or util.py). The idea is that the whole tool should get better at not getting stuck if the database is locked. I don't think it happens as often during normal operation since you aren't running as many database-changing operations at once, but sometimes people execute multiple scans at once and forget to use -u never on them so I imagine database lock errors happen sometimes outside of the test suite.

That suggests another usability improvement: if we do exceed the retries and finally time out, we should have the final error specifically suggest to people that they use -u never on parallel jobs so they know to fix it in future runs.

Thanks for clarifying!

@giacob500
Copy link
Contributor Author

I’ve really missed contributing, and I’m happy to be back after a long journey over the past couple of months! Looking forward to your suggestions to sort this one out so I can move on to the next task. Also excited to join the GSoC 2025 meeting when it comes up and learn more about it!
Thanks, and looking forward to your feedback! 😊

@terriko
Copy link
Contributor

terriko commented Mar 4, 2025

I resolved the merge conflicts and have set the tests to run again, but I think we're close to being able to merge this.

@fil1n
Copy link
Contributor

fil1n commented Mar 8, 2025

Hi, @giacob500. It looks like you are using a new library. Please don't forget to add it to requirements.txt

@Arnavk194
Copy link

Hey @giacob500 are you still working on this issue?

@giacob500
Copy link
Contributor Author

Hey @giacob500 are you still working on this issue?

Hi, yes I am still working on it... a bit busy with work but I'd like to get to the bottom of it. Thanks for asking, I will keep you in mind if I need a hand!

@giacob500 giacob500 requested a review from terriko March 17, 2025 13:44
@giacob500
Copy link
Contributor Author

@terriko Apologies, please ignore the review request above. I misclicked it

@giacob500
Copy link
Contributor Author

@terriko Hi, I am a bit confused here... Based on the screenshot below you have proposed some changes and I have to approve them? Or the opposite?
image
All that is left for this PR is to make the Linting checks pass or there is more that needs to be done?
After I resume the context of what needs to be done to complete this pull request I will

add the new library to the requirements.txt

as suggested by @fil1n in a previous comment, then commit.
Thanks

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

Successfully merging this pull request may close these issues.

4 participants