-
Notifications
You must be signed in to change notification settings - Fork 130
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
Assertions refactoring #1566
Assertions refactoring #1566
Conversation
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.
[question to reviewers] In original framework we had assertSum method with overloaded version accepting isMonotonic parameter added. This was little unclear to me for two reasons:
It was not obvious that assertSum without isMonotonic parameter was the same as assertSum(isMonotonic=true)
In YAML we used instrument types gauge, counter, updowncounter, while in assertions we used assertGauge, assertSum with and without isMonotonic parameter.
That's why I decided to implement assertGauge, assertCounter and assertUpDownCounter method families here.
I know it may lead to more methods to be implemented but for me this new API is cleaner.
What do you think about this approach?
Method names improvement.
refactor with custom assertj assertions
Minor refactorings, mostly renaming. Fixed typos.
/* | ||
List<String> gcLabels = | ||
Arrays.asList( | ||
"Code Cache", | ||
"PS Eden Space", | ||
"PS Old Gen", | ||
"Metaspace", | ||
"Compressed Class Space", | ||
"PS Survivor Space"); | ||
List<String> gcCollectionLabels = Arrays.asList("PS MarkSweep", "PS Scavenge"); | ||
*/ |
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.
leftover from my PR, we should remove it
@@ -125,6 +125,8 @@ void endToEndTest(@TempDir Path tmpDir) { | |||
verifyMetrics(); | |||
} | |||
|
|||
// TODO: This implementation is DEPRECATED and will be removed once all integration tests are | |||
// migrated to MetricsVerifier | |||
protected void waitAndAssertMetrics(Iterable<Consumer<Metric>> assertions) { |
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.
[for reviewer] we can't use @Deprecated
here without introducing compiler warnings and having to use @SuppressWarnings
in test code, so migrating away from this will be done in multiple steps as part of #1573
Preview of new metrics assertions framework.
Assertion errors are now reported in much more compact and descriptive form.
Error message always contain only data related to failing metric, as opposed to old assertions where all received metrics were listed and it was extremely hard to find what is wrong in large set of metrics.