You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "FredrikAnderson (via GitHub)" <gi...@apache.org> on 2024/08/09 13:29:57 UTC
[PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
FredrikAnderson opened a new pull request, #768:
URL: https://github.com/apache/maven-surefire/pull/768
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SUREFIRE) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] Format the pull request title like `[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `SUREFIRE-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean install` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] You have run the integration tests successfully (`mvn -Prun-its clean install`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#discussion_r1731624164
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/report/LegacyPojoStackTraceWriter.java:
##########
@@ -54,7 +52,7 @@ public String writeTraceToString() {
if (isMultiLineExceptionMessage(t)) {
// SUREFIRE-986
String exc = t.getClass().getName() + ": ";
- if (StringUtils.startsWith(builder, exc)) {
+ if (builder.toString().startsWith(exc)) {
Review Comment:
I think you could use `builder.indexOf(exc) == 0` here to avoid creating the string
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2305505361
> Can any dependency be removed?
No.
apache commons-lang3 still used in module, surefire-its.
StringUtils class still used alot in module surefire-api.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#discussion_r1819711568
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/report/LegacyPojoStackTraceWriter.java:
##########
@@ -54,7 +52,7 @@ public String writeTraceToString() {
if (isMultiLineExceptionMessage(t)) {
// SUREFIRE-986
String exc = t.getClass().getName() + ": ";
- if (StringUtils.startsWith(builder, exc)) {
+ if (builder.toString().startsWith(exc)) {
Review Comment:
@FredrikAnderson
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2543996705
Some artifact seems already uploaded and can't be again!? Seems more build environment related to me than cause specifically by this change.
Don't know how to proceed here, I can't retrigger build. Please advice.
Error:
With the provided path, there will be 7959 files uploaded
Artifact name is valid!
Root directory input is valid!
Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2444408277
@FredrikAnderson Can you check the failures please?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2544034839
Looks like it's simply failing to me:
```
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at org.apache.maven.surefire.its.JUnit47RedirectOutputIT.checkReports(JUnit47RedirectOutputIT.java:51)
at
```
Am I missing something?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2277953892
Removed usage of StringUtils from:
* Apache Commons Lang StringUtils
* Surefire API StringUtils
Now only using Surefire Shared StringUtils. Is this enough for this ticket or are we aiming for something else?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#discussion_r1718500008
##########
surefire-its/src/test/java/org/apache/maven/surefire/its/JUnit47RedirectOutputIT.java:
##########
@@ -46,11 +45,11 @@ public void testClassesParallel() {
}
private void checkReports(OutputValidator validator) {
- String report = StringUtils.trimToNull(validator
+ String report = trimToNull(validator
.getSurefireReportsFile("junit47ConsoleOutput.Test1-output.txt")
.readFileToString());
assertNotNull(report);
- String report2 = StringUtils.trimToNull(validator
+ String report2 = trimToNull(validator
Review Comment:
This is test code. The output and input should be fully reproducible as either null or not null, so we likely don't need any of this empty to null conversion here. Depending on what the output is either simply remove trimToNull, or remove trimToNull and expect an empty string instead of null. Much simpler.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2544012227
What?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2416524549
Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2544014079
Tried to resolve issues which has been commented and committed them. Guess someone need to approve the latest changes and PR?
Futher the build fails for some unknown reason, which is what I commented before. Can't start retry/rebuild myself if that would work better now, so would need someone else to do that if you think that is a good option to try?
Only way I see I can take some action myself is to add an unneeded commit to trigger the build, which I would not prefer.
Open for advice how to proceed with this PR.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on code in PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#discussion_r1819728131
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/report/LegacyPojoStackTraceWriter.java:
##########
@@ -54,7 +52,7 @@ public String writeTraceToString() {
if (isMultiLineExceptionMessage(t)) {
// SUREFIRE-986
String exc = t.getClass().getName() + ": ";
- if (StringUtils.startsWith(builder, exc)) {
+ if (builder.toString().startsWith(exc)) {
Review Comment:
Yes changed to proposed statement: if (builder.indexOf(exc) == 0)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#discussion_r1732784333
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/report/LegacyPojoStackTraceWriter.java:
##########
@@ -54,7 +52,7 @@ public String writeTraceToString() {
if (isMultiLineExceptionMessage(t)) {
// SUREFIRE-986
String exc = t.getClass().getName() + ": ";
- if (StringUtils.startsWith(builder, exc)) {
+ if (builder.toString().startsWith(exc)) {
Review Comment:
Correct. That should be it.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2425147181
@FredrikAnderson Care to complete this one?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on code in PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#discussion_r1819728131
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/report/LegacyPojoStackTraceWriter.java:
##########
@@ -54,7 +52,7 @@ public String writeTraceToString() {
if (isMultiLineExceptionMessage(t)) {
// SUREFIRE-986
String exc = t.getClass().getName() + ": ";
- if (StringUtils.startsWith(builder, exc)) {
+ if (builder.toString().startsWith(exc)) {
Review Comment:
Yes changed to proposed statement: if (builder.toString().startsWith( ...
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2442563390
> @FredrikAnderson Care to complete this one?
Yes. Tried by triggering a build
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#issuecomment-2288948655
Can any dependency be removed?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on code in PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#discussion_r1727755901
##########
surefire-its/src/test/java/org/apache/maven/surefire/its/JUnit47RedirectOutputIT.java:
##########
@@ -46,11 +45,11 @@ public void testClassesParallel() {
}
private void checkReports(OutputValidator validator) {
- String report = StringUtils.trimToNull(validator
+ String report = trimToNull(validator
.getSurefireReportsFile("junit47ConsoleOutput.Test1-output.txt")
.readFileToString());
assertNotNull(report);
- String report2 = StringUtils.trimToNull(validator
+ String report2 = trimToNull(validator
Review Comment:
Agree. Code adjusted and pushed.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [SUREFIRE-2172] StringUtils: yet more of them [maven-surefire]
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz commented on code in PR #768:
URL: https://github.com/apache/maven-surefire/pull/768#discussion_r2807297972
##########
surefire-its/src/test/java/org/apache/maven/surefire/its/JUnit47RedirectOutputIT.java:
##########
@@ -46,14 +45,14 @@ public void testClassesParallel() {
}
private void checkReports(OutputValidator validator) {
- String report = StringUtils.trimToNull(validator
+ String report = validator
.getSurefireReportsFile("junit47ConsoleOutput.Test1-output.txt")
- .readFileToString());
- assertNotNull(report);
- String report2 = StringUtils.trimToNull(validator
+ .readFileToString();
+ assertTrue(report.isEmpty());
Review Comment:
`assertNotNull(StringUtils.trimToNull)` -> `assertFalse(report.isEmpty)`
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org