[SUREFIRE-1727] junitplatform: handle all containers #257

t-8ch · 2019-11-27T14:49:04Z

Junit 5 can nest test containers arbitrarily deep.
They also do not have to relate to classes or methods.

sormuras · 2019-11-27T14:52:03Z

Interesting small change set for that generalization. Did you check that reports still work with "all containers", especially those, that are not class/method based?

t-8ch · 2019-11-27T15:09:09Z

It's more of a work in progress.
Currently the reporting is weird, because surefire expects the all entries to be Class/Mathod based.
This will need a policy decision whether surefire will change its internal APIs to be more generic or if there be a more complex mapping from Junit5 concepts to surefire entries.

Tibor17 · 2019-11-27T16:54:00Z

yes surefire will upgrade the ReportEntry with the container name(s) but i hope it is not mandatory in this PR.

Tibor17 · 2019-11-27T16:57:59Z

Which containers are not class/method based? Do we have those in our integration tests?
If not, let's add those.

Tibor17 · 2019-11-27T17:01:44Z

The ReportEntry does not have the terminology like class nor method. It has getSourceName and getName, so it can be really anything, any source, even a script or cucumber feature or spec.

Tibor17 · 2019-11-27T17:11:29Z

...latform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java

@@ -410,11 +410,19 @@ public void allDiscoveredTestsAreInvokedForNullArgument()
        InOrder inOrder = inOrder( runListener );

        ArgumentCaptor<SimpleReportEntry> report = ArgumentCaptor.forClass( SimpleReportEntry.class );
-        inOrder.verify( runListener )
+        inOrder.verify( runListener, times( 2 ) )


What was the second call of testSetStarting?
I just want to understand it.

The engine container.
In theory one testplan can contain both the normal Jupiter engine and also the "Vintage" engine (which will execute Junit4 tests). Also more engines can be written by users.

Tibor17 · 2019-11-27T17:14:04Z

...latform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java

+
+        SimpleReportEntry engine = report.getAllValues().get( 0 );
+        Assertions.assertThat( engine.getSourceName() )
+            .isEqualTo( "JUnit Jupiter" );


Due to this we should chceck the XML reports.
This may appear in the report and that's wrong because it is an engine and not a class.
We have another issue for the Cucumber where the ReportEntry has to be updated.
But still not sure if this is needed to transfer engine name in this issue we have here.

t-8ch · 2019-11-27T17:15:18Z

Which containers are not class/method based? Do we have those in our integration tests?
If not, let's add those.

All subclasses of TestSource.
And I think in theory its possible to discover your tests and containers completely dynamically, without any backing source.

The ReportEntry does not have the terminology like class nor method. It has getSourceName and getName, so it can be really anything, any source, even a script or cucumber feature or spec.

Ah, yes that makes sense. I somehow interpreted it as sourcefile

Tibor17 · 2019-11-27T17:18:05Z

@t-8ch
but your requirement to support all engines is covered already in https://issues.apache.org/jira/browse/SUREFIRE-1724
I thought that you want to fix only Failures during test template creation are ignored as it is in JIRA.
So now i do not know why we have another title in this PR with same Jira ID.

t-8ch · 2019-11-27T17:21:41Z

@Tibor17 The problem is that currently there is no entity in surefire to report the error on.
The error is generated by a container. But surefire only cares about errors from tests.
Also it has no way of representing a test that does not belong to a class.

So the idea was to first expose all containers to surefire and then allow the reporting of errors on them.

t-8ch · 2019-11-27T17:23:56Z

I fear that even if we find a specific workaround for this one case by for example reporting it onto the class instead of the factory method this will break down as soon as more exotic test hierarchies are used.

(Also I will have to read the ticket you linked)

Tibor17 · 2019-11-27T17:24:31Z

Now i know what you mean but this is the same in JUnit4.
It works there.
Suppose that the Runner of JUnit4 fails and the test constructor fails too.
See the JUnit4Provider, there we fire an event with the only testFailed with class name but no method name. I think this would be the way. Maybe not nice but having the unit tests will guide us to make it better in SUREFIRE-1724. My guess with the workflow.

t-8ch · 2019-11-27T17:28:46Z

Ah, ok. This make sense. I'll try this tomorrow.

Tibor17 · 2019-11-27T17:29:58Z

@t-8ch
I want to make sure that we are on the same line. Is Cucumber exotic hierarchy for you?
If it is then we can make a workaround here and then a good and complex fix in SUREFIRE-1724.

Tibor17 · 2019-11-27T17:32:53Z

Maybe keep this PR for the future motivation in SUREFIRE-1724 and open a new one for SUREFIRE-1727. oki?

Junit 5 can nest test containers arbitrarily deep. They also do not have to relate to classes or methods.

t-8ch · 2019-11-27T17:35:33Z

@Tibor17 I just overrode the PR with a new patch. WDYT?
If it looks ok, I'll add tests tomorrow.

Tibor17 · 2019-11-27T17:39:50Z

no problem.
Question: this change you have made includes also the name of method too? So when I see the native report in your JIRA description of the issue there is also the method.

Tibor17 · 2019-11-28T02:38:33Z

@t-8ch
We have a similar issue SUREFIRE-1688 which happens when the method annotated by @BeforeAll throws an exception.
It is similar to this issue but it is not identical.
You can leave your comment, maybe you have opinion about the change needed. It looks simple.
IMO the fix should be placed at the line so I would like to work on it and satisfy given unit test in the pullrequest #255 .

t-8ch · 2019-11-28T09:47:48Z

@Tibor17

We have a similar issue SUREFIRE-1688 which happens when the method annotated by @BeforeAll throws an exception.

It looks mostly the same, only that for my issue the source of the container is a MethodSource while there it is a ClassSource.

IMO the fix should be placed at the line

This would not work for my case, as it is neither a class nor a test.
This line should not be reached because the failed entity is only a container and not a test

Tibor17 · 2019-11-28T10:23:37Z

@t-8ch
no no, it is a class with @BeforeAll. Feel free to add a test in this PR. I will open new PR for the second distinct issue because i think these are two different.

Additionally, I tried to avoid if ( isClass || isTest ) successfully and the tests JUnitPlatformProviderTest pass successfully where three tests failed in RunListenerAdapterTest because the tests missed the Type but that's the third distinct PR to open.

t-8ch · 2019-11-28T12:35:42Z

"Mostly the same" was from the "RunListener" perspective. Both are failed containers.
I have two new platform integration tests like #255 (for TestTemplate and TestFactory)
Do you want me to override this PR or open a new one?

Tibor17 · 2019-12-02T00:52:38Z

@t-8ch
Do you have time for this? How are you doing with the tests?

t-8ch · 2019-12-02T11:31:20Z

I will take another look at it.
In the meantime the tests are here: https://github.com/t-8ch/maven-surefire/tree/SUREFIRE-1727-3

Tibor17 · 2020-01-24T23:25:26Z

Closed in favor of #267 .

t-8ch changed the title ~~[SUREFIRE-1727] junitplatform: handle all containers~~ WIP: [SUREFIRE-1727] junitplatform: handle all containers Nov 27, 2019

Tibor17 changed the title ~~WIP: [SUREFIRE-1727] junitplatform: handle all containers~~ [SUREFIRE-1727] junitplatform: handle all containers Nov 27, 2019

Tibor17 reviewed Nov 27, 2019

View reviewed changes

[SUREFIRE-1727] junitplatform: handle all containers

b17e821

Junit 5 can nest test containers arbitrarily deep. They also do not have to relate to classes or methods.

t-8ch force-pushed the SUREFIRE-1727 branch from f95ca4f to b17e821 Compare November 27, 2019 17:35

Tibor17 mentioned this pull request Nov 29, 2019

[SUREFIRE-1688] Add failing test case for failure in BeforeAll method #258

Merged

ajohnstonTE mentioned this pull request Jan 24, 2020

[SUREFIRE-1741] JUnit5: Detect failed containers #267

Merged

Tibor17 closed this Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SUREFIRE-1727] junitplatform: handle all containers #257

[SUREFIRE-1727] junitplatform: handle all containers #257

t-8ch commented Nov 27, 2019

sormuras commented Nov 27, 2019

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

Tibor17 Nov 27, 2019

t-8ch Nov 27, 2019

Tibor17 Nov 27, 2019

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

t-8ch commented Nov 27, 2019 •

edited

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019 •

edited

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019 •

edited

Tibor17 commented Nov 27, 2019 •

edited

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

Tibor17 commented Nov 28, 2019

t-8ch commented Nov 28, 2019

Tibor17 commented Nov 28, 2019 •

edited

t-8ch commented Nov 28, 2019 •

edited

Tibor17 commented Dec 2, 2019

t-8ch commented Dec 2, 2019

Tibor17 commented Jan 24, 2020

[SUREFIRE-1727] junitplatform: handle all containers #257

[SUREFIRE-1727] junitplatform: handle all containers #257

Conversation

t-8ch commented Nov 27, 2019

sormuras commented Nov 27, 2019

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

Tibor17 Nov 27, 2019

Choose a reason for hiding this comment

t-8ch Nov 27, 2019

Choose a reason for hiding this comment

Tibor17 Nov 27, 2019

Choose a reason for hiding this comment

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

t-8ch commented Nov 27, 2019 • edited

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019 • edited

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019 • edited

Tibor17 commented Nov 27, 2019 • edited

t-8ch commented Nov 27, 2019

Tibor17 commented Nov 27, 2019

Tibor17 commented Nov 28, 2019

t-8ch commented Nov 28, 2019

Tibor17 commented Nov 28, 2019 • edited

t-8ch commented Nov 28, 2019 • edited

Tibor17 commented Dec 2, 2019

t-8ch commented Dec 2, 2019

Tibor17 commented Jan 24, 2020

t-8ch commented Nov 27, 2019 •

edited

Tibor17 commented Nov 27, 2019 •

edited

Tibor17 commented Nov 27, 2019 •

edited

Tibor17 commented Nov 27, 2019 •

edited

Tibor17 commented Nov 28, 2019 •

edited

t-8ch commented Nov 28, 2019 •

edited