Fix node versions #3286
Fix node versions #3286
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
This test fails intermittently even on normal CI runs. I've thought about looking into this more but haven't had a chance. An ideal solution would be event based rather than timer based so we don't have these failures that just aren't guaranteed to work that well. Or explore using |
|
Well, the weird thing is that it keeps failing the VM nightly tests, see: https://github.com/ethereumjs/ethereumjs-monorepo/actions/workflows/vm-nightly-test.yml So this is not a random event. I had it passing locally but it keeps failing on CI, so yes, I think we should go with your suggestion and make the test more robust. (I will not have time to address this today) |
|
Thanks for giving this a start! 🙏 |
|
In looking further at this test, this looks like a prime candidate for moving from |
jochem-brouwer
added
PR state: needs review
PR state: WIP
and removed
PR state: WIP
PR state: needs review
labels
jochem-brouwer
force-pushed
the
fix-node-versions
branch
from
49e1f8b to
0175f87
Compare
| @@ -12,7 +12,8 @@ class FetcherTest extends Fetcher<any, any, any> { | |||
| return res | |||
| } | |||
| async request(_job: any, _peer: any) { | |||
| return | |||
| console.trace(_job) | |||
There was a problem hiding this comment.
Just to make sure: is this console.trace() meant to stay here (is this only triggered in the failure case for debugging e.g.?)?
There was a problem hiding this comment.
(Otherwise: cool 🤩 and congrats on the merge! 🙏)
Closes #3279
TODO
Note: the client test seems to rely upon the internal timing of the NodeJS timers. I think if we make this longer (I made it 100 times longer) then we are ok. (Although this raises the question if we should take a deeper look if we can improve this test)