Skip to content
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

Merged
merged 4 commits into from Dec 14, 2022

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Dec 14, 2022

Only one test is left -- the multithreaded one.

@maloel maloel requested a review from Nir-Az December 14, 2022 11:31
auto not_empty = []( stabilized_value const & self ) {
return ! self.empty();
};
auto to_string = []( stabilized_value const & self ) -> std::string {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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()
Copy link
Collaborator

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]")


Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

@Nir-Az Nir-Az left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maloel maloel merged commit f2b2434 into IntelRealSense:development Dec 14, 2022
@maloel maloel deleted the stabilized branch December 14, 2022 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants