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

tests: merge test_metrics.py into prometheus_test.py #2254

Merged
merged 2 commits into from
Jun 10, 2024

Conversation

tchaikov
Copy link
Contributor

in this series, we move all tests in test_metrics.py into prometheus_test.py to drop the repeating code.

@tchaikov tchaikov requested a review from amnonh May 19, 2024 11:36
@tchaikov
Copy link
Contributor Author

tchaikov commented May 19, 2024

@amnonh this is a follow-up of #2237. could you take a look? i tested it with prometheus 2.52.0 .

@tchaikov tchaikov force-pushed the prometheus-test branch 3 times, most recently from 3083a2d to 903e2b6 Compare May 19, 2024 23:54
tchaikov added 2 commits May 20, 2024 08:08
in order to reduce the replicated code, this change adds a test to
prometheus_test.py, to verify the metrics without aggregations.

since we already have the coverage for this feature, let's drop it
from test_metrics.py.

Signed-off-by: Kefu Chai <[email protected]>
in this change, we

* add a test for testing protobuf support in prometheus_test.py
* drop test_metrics.py, as all the tests in it have been moved into
  prometheus_test.py

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov
Copy link
Contributor Author

@amnonh hello Amnon, ping?

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 5, 2024

@amnonh hey Amnon, ping?

@amnonh
Copy link
Contributor

amnonh commented Jun 5, 2024

@tchaikov I was on vacation, I'll take a look

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 5, 2024

Thank you Amnon!

@amnonh
Copy link
Contributor

amnonh commented Jun 6, 2024

In general, it makes sense; that was my original comment when you added prometheus_test.py; why have two files for the same purpose when we can have one?

I haven't tested the code but I assume it works

Copy link
Contributor

@amnonh amnonh left a comment

Choose a reason for hiding this comment

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

LGTM

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 7, 2024

Amnon, thank you for your review and approval.

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 7, 2024

@scylladb/seastar-maint hello maintainers, could you help merge this change?

@xemul xemul closed this in 71d9363 Jun 10, 2024
@xemul xemul merged commit 71d9363 into scylladb:master Jun 10, 2024
12 checks passed
@tchaikov tchaikov deleted the prometheus-test branch June 10, 2024 22:28
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.

3 participants