-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
stabilized-value in pyrsutils; replace 9 cpp tests with 1 py #11214
Conversation
auto not_empty = []( stabilized_value const & self ) { | ||
return ! self.empty(); | ||
}; | ||
auto to_string = []( stabilized_value const & self ) -> std::string { |
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.
those 2 functions are global will generic names but have a specific implementation.
What will happen the next time you want to add them to a different utility?
Can you use some namespace or rename to a more specific var names?
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.
Global? No, they're just in this code.
If needed, we'll rename or move inline.
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 meant that it's not binds to the stabilized value utility, it is known in all of pyrsutils
test.finish() | ||
# | ||
############################################################################################# | ||
test.print_results_and_exit() |
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.
IMO 2 tests are missing:
TEST_CASE("update stable value - last stable not in history", "[stabilized value]")
TEST_CASE("update stable value - last stable is in history", "[stabilized value]")
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 didn't see them in the cpp files... you see them?
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.
Oh I think ,aybe you're right, I'll add
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.
Done
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.
LGTM
Only one test is left -- the multithreaded one.