-
Notifications
You must be signed in to change notification settings - Fork 39
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
Introduce dropwizard/metrics
integration test
#894
Conversation
Looks good. No mutations were possible for these changes. |
dropwizard/metrics
integration test
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.
Try to make sure it runs for Java 11 and 17. We discussed this offline :).
Some comments but in general this looks really nice!
private String username = ""; | ||
private String password = ""; | ||
- private Set<MetricAttribute> disabledMetricAttributes = Collections.emptySet(); | ||
+ private Set<MetricAttribute> disabledMetricAttributes = emptySet(); |
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.
Interesting, are we not rewriting this to ImmutableSet.of
?
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.
CollectorMutability
is disabled, because there's a few edge cases iirc. I can supress those edge cases 👍 and re-enable.
@@ -0,0 +1,8 @@ | |||
metrics-core/src/main/java/com/codahale/metrics/CsvReporter.java:[382,36] [FormatStringConcatenation] Defer string concatenation to the invoked method | |||
metrics-graphite/src/main/java/com/codahale/metrics/graphite/GraphiteReporter.java:[429,18] [Slf4jLogStatement] Log statement contains 0 placeholders, but specifies 1 matching argument(s) |
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.
Oh nice, this will be interesting to fix upstream! As this is literally a bug!
(Sorry didn't mean to approve yet 😬.) |
Looks good. No mutations were possible for these changes. |
2 similar comments
Looks good. No mutations were possible for these changes. |
Looks good. No mutations were possible for these changes. |
0ebea1f
to
afe7925
Compare
Looks good. No mutations were possible for these changes. |
064a1cb
to
f16db40
Compare
Looks good. No mutations were possible for these changes. |
2 similar comments
Looks good. No mutations were possible for these changes. |
Looks good. No mutations were possible for these changes. |
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.
Some comments 😄.
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg> | ||
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg> | ||
</compilerArgs> | ||
- <annotationProcessorPaths> |
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.
It doesn't work if we leave this one here combined with the "append" usage in the other profile?
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.
Leaving this here results in a error: plug-in not found: ErrorProne
<compilerArgs> | ||
<arg>-Xlint:all</arg> | ||
<arg>-XDcompilePolicy=simple</arg> | ||
- <arg>-Xplugin:ErrorProne -XepExcludedPaths:.*/target/generated-sources/.*</arg> |
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.
Do we use this? Otherwise I'd say we can leave this untouched to minimize the diff.
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.
Same as above
Looks good. No mutations were possible for these changes. |
/integration-test |
Looks good. No mutations were possible for these changes. |
2 similar comments
Looks good. No mutations were possible for these changes. |
Looks good. No mutations were possible for these changes. |
faea805
to
2ef8268
Compare
Looks good. No mutations were possible for these changes. |
1 similar comment
Looks good. No mutations were possible for these changes. |
47bbaa7
to
65dbef0
Compare
Rebased and resolved conflict. |
Looks good. No mutations were possible for these changes. |
65dbef0
to
123c2bd
Compare
Looks good. No mutations were possible for these changes. |
ed7545c
to
ee00181
Compare
Looks good. No mutations were possible for these changes. |
Looks good. No mutations were possible for these changes. |
Quality Gate passedIssues Measures |
I'm unable to figure out why the refaster options aren't being used in the metrics integration test. |
Looks good. No mutations were possible for these changes. |
Quality Gate passedIssues Measures |
Closing in favor of: #1426. |
No description provided.