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

Finish migration to native pytest #1443

Merged
merged 4 commits into from
Feb 23, 2022
Merged

Conversation

pquentin
Copy link
Member

This is the last PR after #1387, #142, #1426 and #1439. When this and #1439 have been merged, then we won't be using TestCase anymore, which means the test will be much more readable.

@pquentin pquentin added cleanup Linter changes, reformatting, removal of unused code etc. :internal Changes for internal, undocumented features: e.g. experimental, release scripts labels Feb 22, 2022
@pquentin pquentin requested a review from DJRickyB February 22, 2022 12:02
@pquentin pquentin self-assigned this Feb 22, 2022
Copy link
Contributor

@DJRickyB DJRickyB left a comment

Choose a reason for hiding this comment

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

LGTM! (looks GREAT to me)


self.assertAlmostEqual(50, self.metrics_store.get_mean("query_latency"))
assert round(abs(50 - self.metrics_store.get_mean("query_latency")), 7) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

seems fine, but for my programming knowledge is there a reason for 7 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just assertAlmostEqual default behavior. I should switch those assert round(abs(...)) and assert math.isclose(...) to use pytest.approx() in a follow up PR.

@pquentin
Copy link
Member Author

Thank you for the kind words!

@pquentin pquentin merged commit e382ba9 into elastic:master Feb 23, 2022
@pquentin pquentin deleted the pytest-migration branch February 23, 2022 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Linter changes, reformatting, removal of unused code etc. :internal Changes for internal, undocumented features: e.g. experimental, release scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants