You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by GitBox <gi...@apache.org> on 2022/04/12 01:23:09 UTC
[GitHub] [jmeter] wkurniawan07 opened a new pull request, #708: Add necessary fixes for passing CI build
wkurniawan07 opened a new pull request, #708:
URL: https://github.com/apache/jmeter/pull/708
## 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`](http://errorprone.info/docs/inlineme) are reported in almost all places where `@Deprecated` annotation is used, thus suppressed.
- `JavaUtilDate` is suppressed, otherwise all instances of `Date` has 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 7d2afdc5eca3b9d6847712f79730fcac4c8a99db. 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.
## Types of changes
- Bug fix (non-breaking change which fixes an issue)
## Checklist:
- [x] My code follows the [code style][style-guide] of this project.
- [x] I have updated the documentation accordingly.
[style-guide]: https://wiki.apache.org/jmeter/CodeStyleGuidelines
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848009517
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
Does this really work? Is there any configuration that enforces that `SystemCommand` outputs the data in UTF-8?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] FSchumacher commented on pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
FSchumacher commented on PR #708:
URL: https://github.com/apache/jmeter/pull/708#issuecomment-1098118302
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] FSchumacher commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
FSchumacher commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848657005
##########
src/protocol/tcp/src/main/java/org/apache/jmeter/protocol/tcp/sampler/TCPClientImpl.java:
##########
@@ -115,11 +116,15 @@ public String read(InputStream is, SampleResult sampleResult) throws ReadExcepti
// do we need to close byte array (or flush it?)
if(log.isDebugEnabled()) {
- log.debug("Read: {}\n{}", w.size(), w.toString());
+ log.debug("Read: {}\n{}", w.size(), w.toString(CHARSET));
}
return w.toString(CHARSET);
} catch (IOException e) {
- throw new ReadException("Error reading from server, bytes read: " + w.size(), e, w.toString());
+ 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);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848014117
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/HttpRequestHdr.java:
##########
@@ -189,7 +189,7 @@ public byte[] parse(InputStream in) throws IOException {
if (log.isDebugEnabled()){
log.debug("rawPostData in default JRE encoding: {}, Request: '{}'",
new String(rawPostData, Charset.defaultCharset()),
- clientRequest.toString().replaceAll("\r\n", CRLF));
+ clientRequest.toString(Charset.defaultCharset().name()).replaceAll("\r\n", CRLF));
Review Comment:
`StandardCharsets.ISO_8859_1` seems to be more appropriate here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848697699
##########
build.gradle.kts:
##########
@@ -403,6 +403,8 @@ allprojects {
disable(
"ComplexBooleanConstant",
"EqualsGetClass",
+ "InlineMeSuggester",
+ "JavaUtilDate",
Review Comment:
Currently the code used ` @SuppressWarnings("JdkObsolete")`, so I would rather suggest replacing it with `@SuppressWarnings("JavaUtilDate")`.
Then we would know if when usages of Date appear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848698042
##########
build.gradle.kts:
##########
@@ -403,6 +403,8 @@ allprojects {
disable(
"ComplexBooleanConstant",
"EqualsGetClass",
+ "InlineMeSuggester",
+ "JavaUtilDate",
Review Comment:
In other words, I would avoid adding `JavaUtilDate` to `disable...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848172847
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
They are fine with ISO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848043421
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
The command in question is created in `org.apache.jmeter.gui.action.HtmlReportGenerator#createGenerationCommand`, and it is like `java -jar ...`.
Apparently, we could add `-Dfile.encoding=UTF-8`, and then we would have reasons to decode the output via UTF-8.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] wkurniawan07 commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
wkurniawan07 commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848118079
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
That doesn't really sound like UTF-8 is applicable here for all cases. Probably should just use default charset, since that is the default behaviour of `.toString()` (without arg) anyway?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] FSchumacher commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
FSchumacher commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848751306
##########
build.gradle.kts:
##########
@@ -403,6 +403,8 @@ allprojects {
disable(
"ComplexBooleanConstant",
"EqualsGetClass",
+ "InlineMeSuggester",
+ "JavaUtilDate",
Review Comment:
Will submit a patch in a second, and then look into replacing the formatters with newer ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] FSchumacher commented on pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
FSchumacher commented on PR #708:
URL: https://github.com/apache/jmeter/pull/708#issuecomment-1097725078
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848160635
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
I'm fine if the changes are made in separate PRs. Then the first PR should be like "replace .toString() with .toString(..defaultCharset...) to make it explicit where default charset is used"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] wkurniawan07 commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
wkurniawan07 commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848014545
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
Can't really say. What I can say is there is a not small amount of code in here that assumes `UTF8`. Or should this fall back to `Charset.defaultCharset()` instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848136633
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
Replacing `.toString()` with `.toString(...defaultCharset...)` is a step forward to identifying the locations that rely on default charset. However, it would be great if we can move towards UTF-8 since UTF-8 will be the default in the upcoming Java.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] asfgit closed pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #708: Add necessary fixes for passing CI build
URL: https://github.com/apache/jmeter/pull/708
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] wkurniawan07 commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
wkurniawan07 commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848144853
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
Taking an idealistic point-of-view, I would agree with you. However, you're the one who knows the possible complications and what-nots if we do use UTF-8 (or anything else, for that matter) here right now. So should this stay as UTF-8?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848013952
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/HttpRequestHdr.java:
##########
@@ -164,7 +164,7 @@ public byte[] parse(InputStream in) throws IOException {
inHeaders = false;
firstLine = false; // cannot be first line either
}
- final String reqLine = line.toString();
+ final String reqLine = line.toString(Charset.defaultCharset().name());
Review Comment:
Only ASCII is should be in the headers (see `CharUtils.isAscii` above), so this encoding should be like `StandardCharsets.ISO_8859_1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] wkurniawan07 commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
wkurniawan07 commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848014545
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
Can't really say. What I can say is there is a not small amount of code in here (the repo, not the class) that assumes `UTF8`. Or should this fall back to `Charset.defaultCharset()` instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] wkurniawan07 commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
wkurniawan07 commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848171030
##########
src/core/src/main/java/org/apache/jmeter/gui/action/HtmlReportGenerator.java:
##########
@@ -83,7 +84,7 @@ public List<String> run() {
LOGGER.debug("Running report generation");
resultCode = sc.run(generationCommand);
if (resultCode != 0) {
- errorMessageList.add(commandExecutionOutput.toString());
+ errorMessageList.add(commandExecutionOutput.toString(StandardCharsets.UTF_8.name()));
Review Comment:
Got it. How about the ones in `HttpRequestHdr`? They seem to be unambiguously `ISO_8859_1`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] wkurniawan07 commented on a diff in pull request #708: Add necessary fixes for passing CI build
Posted by GitBox <gi...@apache.org>.
wkurniawan07 commented on code in PR #708:
URL: https://github.com/apache/jmeter/pull/708#discussion_r848992665
##########
src/protocol/tcp/src/main/java/org/apache/jmeter/protocol/tcp/sampler/TCPClientImpl.java:
##########
@@ -115,11 +116,15 @@ public String read(InputStream is, SampleResult sampleResult) throws ReadExcepti
// do we need to close byte array (or flush it?)
if(log.isDebugEnabled()) {
- log.debug("Read: {}\n{}", w.size(), w.toString());
+ log.debug("Read: {}\n{}", w.size(), w.toString(CHARSET));
}
return w.toString(CHARSET);
} catch (IOException e) {
- throw new ReadException("Error reading from server, bytes read: " + w.size(), e, w.toString());
+ 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);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org