You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/01/24 10:39:09 UTC
[GitHub] [maven-surefire] ajohnstonTE opened a new pull request #267:
[SUREFIRE-1741] JUnit5: Detect failed containers
ajohnstonTE opened a new pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267
Failed JUnit5 containers will be reported as test failures. This is intended to address [SUREFIRE-1741](https://issues.apache.org/jira/browse/SUREFIRE-1741).
Note: I can't comment on the JIRA issue currently, but I will when I get in to work.
I just now noticed there's already a PR open that (in my opinion) addresses this issue better and more simply (https://github.com/apache/maven-surefire/pull/257), but since I already wrote it up I'm just gonna open this one anyways.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r372696852
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
Alright, can you start writing the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r373231324
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
yes, you can take it as a motivation with one exception. Pls do not reuse the existing directory `junit-platform-engine-jupiter`. Create your own. The reason is that this directory already contains some tests
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r370920432
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
I'll switch to a static import, but I do need the null check. The method directly below this one passes a null value for a `testExecutionResult` (which ultimately makes its way over to here): https://github.com/apache/maven-surefire/pull/267/files/38ad13f5a6a628282064d1b0976e1ea69a135b0e#diff-ad07b0c74885ecb3d687dcc85495e390R198-R201
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on issue #267: [SUREFIRE-1741]
JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on issue #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#issuecomment-580807711
I am traveling right now, so i will check it in several hours.
Dňa pi 31. 1. 2020, 10:33 Albert Johnston <no...@github.com>
napísal(a):
> *@ajohnstonTE* commented on this pull request.
> ------------------------------
>
> In
> surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
> <https://github.com/apache/maven-surefire/pull/267#discussion_r373387825>:
>
> > @@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
> stw, elapsedTime, reason, systemProperties );
> }
>
> + private boolean isFailedContainer( TestIdentifier testIdentifier,
> + TestExecutionResult testExecutionResult )
> + {
> + return testIdentifier.isContainer() && testExecutionResult != null
>
> In any event, IT's added for the other issue. That should be everything.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/maven-surefire/pull/267?email_source=notifications&email_token=AAH7ER7C6Q74PN7SKRVV77LRAPV7FA5CNFSM4KLEEYEKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTY3DMI#discussion_r373387825>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAH7ERY7I4UHOOT4QIVHVKDRAPV7FANCNFSM4KLEEYEA>
> .
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r372829756
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
IT added for all the functionality changes (plus some additional unit tests for the changes regarding errors vs failures).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r370871042
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
The call of callback method with `null` in `TestExecutionListener` does not make sense. Acctually, the Javadoc on `TestExecutionListener` does not say anything about passing the null to the method parameter.
Here static import of whole `TestExecutionResult.Status.FAILED` whould make the line brief.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r371430596
##########
File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/RunListenerAdapterTest.java
##########
@@ -425,7 +425,7 @@ public void notifiedOfContainerFailure()
throws Exception
{
adapter.executionFinished( newContainerIdentifier(), failed( new RuntimeException() ) );
- verify( listener ).testFailed( any() );
+ verify( listener ).testError( any() );
Review comment:
Right. I initially just re-used the existing logical flow when I made my fix, which led to it being a failure, not an error. So I changed that in this latest set of changes. I can add a check [like the one a few lines below my changes](https://github.com/apache/maven-surefire/pull/267/commits/6df45c50e06b396504b0a846a59a60052f0177ff#diff-ad07b0c74885ecb3d687dcc85495e390R136-R137) to see whether or not it's an `AssertionError` if you'd prefer.
I was actually planning on adding that check for my latest commit, but noticed that the `isClass` (previously `!isTest`) doesn't check that, so instead I limited the scope of my changes and decided I'd just leave it for another PR.
`isClass` block:
https://github.com/apache/maven-surefire/blob/6df45c50e06b396504b0a846a59a60052f0177ff/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java#L121-L126
If I do add that `AssertionError` check, should I add it to the `isClass` block as well? Or just the `isContainer` block?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r370871042
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
The call of callback method with `null` in `TestExecutionListener` does not make sense. Eventhough the Javadoc on `TestExecutionListener` does not say anything about passing the null to the method parameter.
Here static import of whole `TestExecutionResult.Status.FAILED` whould make the line brief.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r371011646
##########
File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/RunListenerAdapterTest.java
##########
@@ -425,7 +425,7 @@ public void notifiedOfContainerFailure()
throws Exception
{
adapter.executionFinished( newContainerIdentifier(), failed( new RuntimeException() ) );
- verify( listener ).testFailed( any() );
+ verify( listener ).testError( any() );
Review comment:
You are modifying the old tests?
The reason between error and failure is that failure throws `AssertionError` but the error throws any other exception.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r370877245
##########
File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java
##########
@@ -396,6 +396,43 @@ public void runDisplayNameTest() throws Exception
assertEquals( "73$71 ✔", reportEntries.get( 0 ).getNameText() );
}
+ @Test
Review comment:
this test looks good!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on issue #267:
[SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on issue #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#issuecomment-581331763
Well, I've been trying to look into another issue I noticed while working on this one, but I haven't really gotten anywhere with it. TL;DR: Using JUnit 5's `Assertions.fail()` method with no arguments or an empty string breaks something in Surefire's reporting. Specifically an NPE. I'll open up a ticket for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r372834207
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
The PR that you closed in favor of this one, #257, the owner had gotten started on some additional IT's for their own issue:
https://github.com/apache/maven-surefire/compare/master...t-8ch:e6c3c4fef38ee6a5e5352bec447e047713b7cd0b
Should I copy those IT's into this? I'm not super familiar with `@TestFactory` or `@TestTemplate`, and I'm not sure what the state of those IT's are, but would that be preferable as a means to also resolve that other issue? I did a quick local check after my first couple commits to see if this addresses that issue, and if I remember correctly, it does.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on issue #267:
[SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on issue #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#issuecomment-578384784
Will do
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r372693975
##########
File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/RunListenerAdapterTest.java
##########
@@ -425,7 +425,7 @@ public void notifiedOfContainerFailure()
throws Exception
{
adapter.executionFinished( newContainerIdentifier(), failed( new RuntimeException() ) );
- verify( listener ).testFailed( any() );
+ verify( listener ).testError( any() );
Review comment:
I see you crated the test methods in the commit `136f79ee`. So it's your's, just fine when you modify your's!
Feel free to rename `!isTest` to `isClass`. I don't have a problem with that due to you have extended the branching with `else if ( testIdentifier.isContainer() )`. If the new argument with `isClass` is fully logical and right to do, then use it of course 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r373239830
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
Did I do that? I know the closed PR did that, but I put mine in the surefire-1741 directory like the BeforeAll one did (for its own issue).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r372762971
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
Yep, on 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE edited a comment on issue #267:
[SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE edited a comment on issue #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#issuecomment-581331763
Well, I've been trying to look into another issue I noticed while working on this one, but I haven't really gotten anywhere with it. TL;DR: Using JUnit 5's `Assertions.fail()` method with no arguments or an empty string breaks something in Surefire's reporting. Specifically an NPE. I'll open up a ticket for it.
This is the issue: https://issues.apache.org/jira/browse/SUREFIRE-1748
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r372828613
##########
File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/RunListenerAdapterTest.java
##########
@@ -425,7 +425,7 @@ public void notifiedOfContainerFailure()
throws Exception
{
adapter.executionFinished( newContainerIdentifier(), failed( new RuntimeException() ) );
- verify( listener ).testFailed( any() );
+ verify( listener ).testError( any() );
Review comment:
Ok, this whole case has been updated to reflect the changes I was suggesting. Test failures and errors should be properly respected for every `FAILURE` now (previously a `RuntimeException` would not have been considered a test error for `@BeforeAll`, for example, it would instead be treated as a failure).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 merged pull request #267: [SUREFIRE-1741]
JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 merged pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r373387825
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
In any event, IT's added for the other issue. That should be everything.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r370868639
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -173,8 +175,9 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
{
classText = null;
}
- String methodName = testIdentifier.isTest() ? classMethodName[2] : null;
- String methodText = testIdentifier.isTest() ? classMethodName[3] : null;
+ boolean isFailedContainer = isFailedContainer( testIdentifier, testExecutionResult );
+ String methodName = ( isFailedContainer || testIdentifier.isTest() ) ? classMethodName[2] : null;
Review comment:
You do not have to use brackets in `( isFailedContainer || testIdentifier.isTest() )`. There is no collision between boolean and String in this case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on issue #267: [SUREFIRE-1741]
JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on issue #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#issuecomment-580982060
@ajohnstonTE Thx for contributing!
What's your next plan? Do you want to stay in Surefire and work with us togethter?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] ajohnstonTE commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
ajohnstonTE commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r370925040
##########
File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java
##########
@@ -185,6 +188,13 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier,
stw, elapsedTime, reason, systemProperties );
}
+ private boolean isFailedContainer( TestIdentifier testIdentifier,
+ TestExecutionResult testExecutionResult )
+ {
+ return testIdentifier.isContainer() && testExecutionResult != null
Review comment:
I think I see the confusion. I have some changes to how the logic is applied to address some other cases. Those changes also end up reducing the scope of that null check to only the appropriate places. I'm currently waiting for it to do a full build, so I'll probably push them a while later today. Once that's done (assuming it passes), I'll get started on the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request
#267: [SUREFIRE-1741] JUnit5: Detect failed containers
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #267: [SUREFIRE-1741] JUnit5: Detect failed containers
URL: https://github.com/apache/maven-surefire/pull/267#discussion_r371011646
##########
File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/RunListenerAdapterTest.java
##########
@@ -425,7 +425,7 @@ public void notifiedOfContainerFailure()
throws Exception
{
adapter.executionFinished( newContainerIdentifier(), failed( new RuntimeException() ) );
- verify( listener ).testFailed( any() );
+ verify( listener ).testError( any() );
Review comment:
You are modifying the old tests?
The difference between the error and failure is that failure throws `AssertionError` but the error throws any other exception.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services