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.

Sign up for GitHub

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

Jump to bottom

test_metrics: make it easier to understand failures #939

Merged
merged 7 commits into from
Dec 8, 2022
Merged

test_metrics: make it easier to understand failures #939

merged 7 commits into from
Dec 8, 2022

Conversation

rukai
Copy link
Member

@rukai rukai commented Nov 24, 2022

Possibly a bit overengineered but I just could not figure out what I was supposed to change in the expected data to make the test pass in the old implementation.

This PR gives us:

  • Stronger assertions - we now assert entire keys exist, and in some cases assert on the corresponding value too.
  • Much better error reporting - the assertion messages give the actual output in a format exactly the same as expected string allowing easy comparison between them.

It seems like since the test was originally written the metrics fields within a line have been changed to be alphabetically sorted, this makes our job a lot easier and allows us to assert that entire keys are included in the metrics output.

prereq to: #933

@rukai rukai force-pushed the improve_test_metrics_error_reporting branch from ccffb45 to 9b5c8f9 Compare November 24, 2022 07:11
@rukai rukai requested a review from conorbros November 25, 2022 00:51
@rukai rukai enabled auto-merge (squash) December 7, 2022 22:41
@github-actions
Copy link

github-actions bot commented Dec 7, 2022

1 benchmark regressed. 0 benchmark improved. Please check the benchmark workflow logs for full details: https://github.com/shotover/shotover-proxy/actions/runs/3643446341

                        Change within noise threshold.
redis/active_get        time:   [314.53 µs 322.98 µs 330.32 µs]
                        thrpt:  [3.0274 Kelem/s 3.0961 Kelem/s 3.1793 Kelem/s]
                 change:
                        time:   [+22.432% +25.865% +29.222%] (p = 0.00 < 0.05)
                        thrpt:  [-22.614% -20.550% -18.322%]
                        Performance has regressed.

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

1 benchmark regressed. 0 benchmark improved. Please check the benchmark workflow logs for full details: https://github.com/shotover/shotover-proxy/actions/runs/3643681690

                        Change within noise threshold.
redis/active_get        time:   [340.47 µs 343.54 µs 345.99 µs]
                        thrpt:  [2.8903 Kelem/s 2.9109 Kelem/s 2.9371 Kelem/s]
                 change:
                        time:   [+21.808% +25.042% +27.956%] (p = 0.00 < 0.05)
                        thrpt:  [-21.848% -20.027% -17.904%]
                        Performance has regressed.

@rukai rukai merged commit c6f481f into shotover:main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benbromhead benbromhead benbromhead approved these changes

@conorbros conorbros conorbros approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rukai @benbromhead @conorbros