Add necessary fixes for passing CI build#708
Add necessary fixes for passing CI build#708wkurniawan07 wants to merge 3 commits intoapache:masterfrom
Conversation
2043b77 to
01e0d92
Compare
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java
Outdated
Show resolved
Hide resolved
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/HttpRequestHdr.java
Outdated
Show resolved
Hide resolved
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/HttpRequestHdr.java
Outdated
Show resolved
Hide resolved
01e0d92 to
0862e8e
Compare
FSchumacher
left a comment
There was a problem hiding this comment.
Thanks for the PR.
Might it be better to split it in (at least) two parts? The first part would be to silence the checks for InlineMeSuggester, JavaUtilDate and DefaultCharset plus the fixes for the rest of the failing checks and the updated checksums. The second part would be the changes to the character set usage as Vladimir and you have already discussed.
But if Vladimir is fine with the change, I am fine, too.
And sorry for breaking the CI and not really noticing it.
| try { | ||
| throw new ReadException("Error reading from server, bytes read: " + w.size(), e, w.toString(CHARSET)); | ||
| } catch (UnsupportedEncodingException ue) { | ||
| throw new RuntimeException("Unsupported CHARSET: " + CHARSET, ue); |
There was a problem hiding this comment.
We might want to catch the UnsupportedEncodingException before the IOException, as that might happen earlier, too and the error message might look funny otherwise.
On this line, we could include a bit more information such as the size of w and the fact, that we got an error while reading/decoding stuff.
There was a problem hiding this comment.
Not that I disagree about it, but this catch block here is unavoidable. It is thrown because w.toString(CHARSET) is called from within throw new ReadException(...).
This is avoidable only if the result of w.toString(CHARSET) is accessible within the catch block without calling the said method, but making it happen requires even more non-trivial changes on this method.
and keep them locally Part of #708
0862e8e to
9ee002d
Compare
That is, why I gave the size of w as an example, only :) That should not throw an UnsupportedEncodingException. But, yes, it will get more complex. Another possibility would be to extract the char conversion into a method, which could discard(catch) the exception and return a meaningful error message instead of the encoded message w. |
Replace JdkObsolete with JavaUtilDate where we can't replace usage of new Date() Use Instant and other newer APIs from java.time, where we can replace Date() without changing our API. Part of #708
Replace JdkObsolete with JavaUtilDate where we can't replace usage of new Date() Use Instant and other newer APIs from java.time, where we can replace Date() without changing our API. And while we are here, we can get use some more isEmpty calls. Part of #708
|
Thanks again for the PR, error prone will be happier now and I haven't added any bigger issues with my changes to your code. |
Description
Add necessary fixes such that CI build can pass.
Note that the solution I introduced, particularly for fixing the errorprone violations, are the ones requiring least amount of change, but not necessarily the most ideal:
InlineMeSuggesterare reported in almost all places where@Deprecatedannotation is used, thus suppressed.JavaUtilDateis suppressed, otherwise all instances ofDatehave to be replaced withLocalDateorInstant. Incidentally, all places where the violations exist are annotated with@SuppressWarnings("JdkObsolete").UTF8.Motivation and Context
I notice that the CI build of this project has not been passing for a while now, in particular due to the commit 7d2afdc. A non-passing CI build allowed in development branch can and will hide subtle (and not subtle) system bugs.
How Has This Been Tested?
Green tick in CI build. (sample: https://github.com/wkurniawan07/jmeter/actions/runs/2152075248)
Types of changes
Checklist: