-
Notifications
You must be signed in to change notification settings - Fork 679
Run pebble database tests in Nightly CI Builds #4026
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
base: master
Are you sure you want to change the base?
Conversation
|
@pmikolajczyk41 here is the nightly ci run https://github.com/OffchainLabs/nitro/actions/runs/19438293760 lets see what happens |
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
|
Ok I am doing another run of nightly and will open this PR if tests pass https://github.com/OffchainLabs/nitro/actions/runs/19448297103 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4026 +/- ##
===========================================
+ Coverage 18.02% 31.64% +13.62%
===========================================
Files 388 446 +58
Lines 48612 54681 +6069
===========================================
+ Hits 8760 17303 +8543
+ Misses 38159 34223 -3936
- Partials 1693 3155 +1462 |
|
@pmikolajczyk41 https://github.com/OffchainLabs/nitro/actions/runs/19448297103 looks like the nightly tests pass now |
pmikolajczyk41
left a comment
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.
do I understand correctly, that the tests where we add manually WithDatabase() just won't work with memory db for some reason?
apart from that clarification question, lgtm
you can, it is just there probably isn't a need to unless there is some extremely niche cache |
hmm, so why do we add this line? we will run these tests with pebble in nightly anyway and I thought the ticket assumed running everything with memory db by default |
You should be able to choose what database you need for a test, if does MemoryDB is one of the options |
|
@KolbyML, great work. Sorry it took me so long to review it. Please resolve the merge conflict, and add it to the merge queue. |
4505b52 to
2ab418d
Compare
Replaces #3915 this PR uses a branch from origin instead of my remote branch. I will use the origin branch going forward.
Resolves NIT-3501
This PR
--test-database-engineflag