-
Notifications
You must be signed in to change notification settings - Fork 795
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
Fix unit test rpc.confirmation_info (remove scheduler flush) #3735
Fix unit test rpc.confirmation_info (remove scheduler flush) #3735
Conversation
Removed the need for scheduler flush, which is broken.
nano/rpc_test/rpc.cpp
Outdated
node1->block_processor.flush (); | ||
node1->scheduler.flush (); | ||
ASSERT_FALSE (node1->active.empty ()); | ||
ASSERT_TIMELY (10s, node1->active.empty () == false); |
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.
We've been using 5s
as a timeout for changes to AEC that we expect to see. Not that it matters in its absolute value (the timeout), but since we are kind of reworking these things now, unless there's a reason not to stick to the same value, it's probably better to keep it consistent at least in unit tests that we change if not in old ones as well.
Also, an even less important nitpick, but just thinking out loud in case you decide to do any addressing (otherwise please don't bother changing just this) -- I haven't seen boolVariable == false
or boolVariable == true
usages across the codebase, so again for consistency reasons we might be better off with the existing if (boolVariable)
/ if (!boolVariable)
style.
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.
Personally I find 5 seconds to be a little too low for comfort. But I am OK to follow that pattern if that is what you guys decided. It should probably be OK.
If find this to be more easily readable
ASSERT_TIMELY (10s, node1->active.empty () == false);
than this:
ASSERT_TIMELY (10s, !node1->active.empty ());
The ! character is too easy miss in the middle of a statement.
Although I have no objection to using !
instead of == false
.
Fix itself looks good, couple nitpicks from my side. Other than that, looking at the CI failure and seeing something that I haven't before (not a "usual" test failure):
@clemahieu / @thsfs -- is this something we already know about? |
Removed the need for scheduler flush, which is broken.