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

Add necessary fixes for passing CI build #708

Closed
wants to merge 3 commits into from

Conversation

wkurniawan07
Copy link
Contributor

@wkurniawan07 wkurniawan07 commented Apr 12, 2022

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:

  • InlineMeSuggester are reported in almost all places where @Deprecated annotation is used, thus suppressed.
  • JavaUtilDate is suppressed, otherwise all instances of Date have to be replaced with LocalDate or Instant. Incidentally, all places where the violations exist are annotated with @SuppressWarnings("JdkObsolete").
  • For the charset-related fixes, I tried to look at what charsets are appropriate based on the file content, otherwise I fall back to 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

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.

Copy link
Contributor

@FSchumacher FSchumacher left a comment

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

build.gradle.kts Outdated Show resolved Hide resolved
asfgit pushed a commit that referenced this pull request Apr 12, 2022
asfgit pushed a commit that referenced this pull request Apr 12, 2022
@FSchumacher
Copy link
Contributor

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.

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.

asfgit pushed a commit that referenced this pull request Apr 13, 2022
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
asfgit pushed a commit that referenced this pull request Apr 13, 2022
asfgit pushed a commit that referenced this pull request Apr 13, 2022
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
@asfgit asfgit closed this in d8a6876 Apr 13, 2022
@FSchumacher
Copy link
Contributor

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.

@wkurniawan07 wkurniawan07 deleted the fix-checksum-problem branch April 13, 2022 14:58
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