Skip to content

Conversation

@Frawless
Copy link
Contributor

@Frawless Frawless commented Oct 8, 2025

This PR solves couple of corner cases that wasn't resolved as part of #3194 and also adds IT to cover it.

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean install to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • 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 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
you have to acknowledge this by using the following check-box.

@Tibor17
Copy link
Contributor

Tibor17 commented Oct 8, 2025

Including #3194
This is a never ending story because the Default reporter has wrong logic. I have some branch where this class should be implemented properly. The problem is that this class has an implicit logic with guesses and string description of test method execution...
These workarounds will never work properly because of the reasons I mentioned.
I will find the branch and we can work on it by then.
Maybe maybe ok for now, maybe, but I did not analyse all your commit.

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

Just merge this and we are done
BTW build needs to be fixed
Thanks

@Frawless Frawless marked this pull request as ready for review November 3, 2025 16:21
@Tibor17 Tibor17 self-requested a review November 6, 2025 19:19
@Tibor17
Copy link
Contributor

Tibor17 commented Nov 6, 2025

@olamy
I talked to @Frawless . We think the same, that #3194 was too eagerly pushed, Not only tests were missing but the functionality is hard to understand due to it splits in two commits. Therefore we tried to have only one simple commit.
Hopefully, there are only trivial changes: 3x dependabot and one Javadoc, so we can replay them (in IntelliJ IDEA by cherry pick) again after we are done.
The class in @Frawless 's change will be touched later and therefore it is important to have this feature in a consistent state, means one commit. It will be easy to understand that feature change if it is in one commit and not two (even far away in history).

@Tibor17
Copy link
Contributor

Tibor17 commented Nov 6, 2025

@olamy I think the NPE in the unit test ForkedBooterTest.testThreadDump should be orthogonal to this change. If it is orthogonal, then the question is why it fails right here.

@Tibor17
Copy link
Contributor

Tibor17 commented Nov 6, 2025

@olamy I think the NPE in the unit test ForkedBooterTest.testThreadDump should be orthogonal to this change. If it is orthogonal, then the question is why it fails right here.

For these purposes I need to have an access to the file system of the this run because there are the logs and stacktraces I need for the analysis. Jenkins was good in this, and now I am missing it.

@Frawless Frawless force-pushed the before-all-flakes-it branch from d2ae5ec to ffa782c Compare November 14, 2025 11:01
XMLWriter ppw = new PrettyPrintXMLWriter(new PrintWriter(fw), XML_INDENT, XML_NL, UTF_8.name(), null);

createTestSuiteElement(ppw, testSetReportEntry, testSetStats); // TestSuite
createTestSuiteElement(ppw, testSetReportEntry, classMethodStatistics); // TestSuite
Copy link
Contributor

Choose a reason for hiding this comment

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

Where this TestSetStats come from? Is it the listener event invoked by the test-provider?
Because, logically this TestSetStats makes sense.
Alright, we can accept this, we should consider this as a workaround because the real fix has to be in the Providers and it is not that easy and definitely it is not one PR.
I want to continue with the Roadmap in 3.0.0 milestones which solves multiple issues, and this one too, but it is so complex change that it requires multiple stable PRs promoted one by one.

@Frawless Frawless force-pushed the before-all-flakes-it branch from ffa782c to 36041de Compare November 14, 2025 20:08
Signed-off-by: Jakub Stejskal <[email protected]>

# Conflicts:
#	maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/DefaultReporterFactory.java
#	maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/report/DefaultReporterFactoryTest.java
@Frawless Frawless force-pushed the before-all-flakes-it branch from 36041de to bdf9fe5 Compare November 14, 2025 20:09
@Tibor17 Tibor17 merged commit b4a53de into apache:master Nov 14, 2025
13 of 14 checks passed
@github-actions
Copy link

@Tibor17 Please assign appropriate label to PR according to the type of change.

@github-actions github-actions bot added this to the 3.5.5 milestone Nov 14, 2025
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