-
Notifications
You must be signed in to change notification settings - Fork 10
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
[1.0.4] bump eos-vm submodule to fix spurious access violations in performance_test_basic_read_only
test
#1102
Conversation
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.
Is there anything we can do in the performance_test_basic_read_only test or some other test to make this more likely to fail (before the fix)?
I think so, I will see what I can do. Probably will need to be a specialized test built on read_only_trx_test |
I added a test that should generally reliably fail before this fix. A (failing) demo run with the new two commits for the test applied on top of 1.0 is at https://github.com/AntelopeIO/spring/actions/runs/12801721083 |
endTime = time.time() + testLengthSeconds | ||
# and then run some other ROtrx that should complete successfully | ||
while time.time() < endTime: | ||
results = sendTransaction(tightloopAccountName, 'doit', {"count": 1000000}, opts='--read') #1 million is a good number to always take <10ms |
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.
Though I am a little worried about the sensitivity across runs of this time requirement
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.
and yup this value is no good in CI but great locally. hmmm I will have to think some on what to do. The test is more accurate the larger this number is (without "going over"). So maybe it needs to try and sniff out a good value before starting the test.
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've pushed up some changes to the test I was hoping would get it working in CI but no... still just way too sensitive to the timing. Not sure where to go from here on it.
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 made at least one more change, cec196e, to try and improve the failures, but no luck. I think the test needs to be run with dedicated resources (really I think all the NP/LR tests should run with dedicated resources)
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.
One counter-point to NP/PR tests running in a more dedicated environment is that the chaos-monkey of the current approach has shaken out bugs we would never see in a more stable environment.
Note:start |
Brings in AntelopeIO/eos-vm#30 to resolve test failures seen in #425
TBD how to provide a test for this; it's tricky from a timing standpoint.