[SUREFIRE-2025] Updated abstractions which helps associating systemProperties() with a test context #475

Tibor17 · 2022-02-24T11:21:18Z

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

Make sure there is a JIRA issue 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.
Each commit in the pull request should have a meaningful subject line and body.
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.
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.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

slawekjaranowski · 2022-02-24T13:15:39Z

...fire-api/src/main/java/org/apache/maven/surefire/api/booter/MasterProcessChannelEncoder.java

-    void systemProperties( Map<String, String> sysProps );
+    void systemProperties( Map<String, String> sysProps, RunMode runMode, Long testRunId );


method name is not descriptive .... what does do .. read/write/verify properties?
Now new params - what for?

documentation is needed

@Tibor17 can you add some documentation for this method ... or you have such plan?

I agree and so I have added the Javadoc, pls see the commit 2840bfa.

The testRunId says - this is the test set.
The runMode says - normal mode, or re-run.

slawekjaranowski · 2022-02-24T13:19:46Z

surefire-booter/src/main/java/org/apache/maven/surefire/booter/spi/EventChannelEncoder.java

-    public void systemProperties( Map<String, String> sysProps )
+    public void systemProperties( Map<String, String> sysProps, RunMode rm, Long testRunId )


we already have field with the same name and type runMode
why not use filed from instance?

field runMode sometime is used ... sometime is overridden by method params in this class
line: 389, 419

The fields runMode will be deleted in ForkinRunListener and EventChannelEncoder.
This PR has only prepared changes in one method signature. The implementation of this abstract method comes right after. This PR is only an intermediate step.

You'r right, the runMode as a test status should not be in 3 classes. It should be only in one, e.g. in the:
JUnit4RunListener which implements framework's listener org.junit.runner.notification.RunListener.

ok, so it is next issue to clean up it.

slawekjaranowski · 2022-02-24T13:24:04Z

surefire-api/src/main/java/org/apache/maven/surefire/api/booter/ForkingRunListener.java

-        target.systemProperties( report.getSystemProperties() );
+        target.systemProperties( report.getSystemProperties(), null, null );


In this class we have filed runMode

why not used here?

why is not propagated to target?

I did split the entire work of SUREFIRE-1860 into 5 pieces.
The line you are asking for is in my IDEA:
target.systemProperties( report.getSystemProperties(), report.getRunMode(), report.getTestRunId() );
Both getters will come in the next PR right after, see SUREFIRE-2015

The target is not JUnit's RunListener implementation.
Only the framework's listener may have this data runMode and be "stateful".
The target is a pure encoder, and so it should not have any notion about stateful data regarding test status. It should only receive data, encode it, and send it to a channel.

So what for EventChannelEncoder has runMode field?

runMode field will be removed in the next PR.
This PR only changed some method signatures in the same manner with the previous one.
After this the implementation would come in PR.

Tibor17 · 2022-02-25T11:37:14Z

@slawekjaranowski
Yesterday I have realized that I extended TestSetReportEntry with system properties in the version 2.20.1. See this:

public interface TestSetReportEntry extends ReportEntry
{
    Map<String, String> getSystemProperties();
}

Due to the system properties are sent only when the test set has finished. Pls notice that the XML report contains these system properties from the end of test set. Therefore I made these consequent changes in the last commit c33024e. The method we spoke above is gone and the functionality is intrisic down in the encoder due to the TestSetReportEntry already contains the system props on the event call testSetCompleted(TestSetReportEntry).

…operties() with a test context added Javadoc TestSetReportEntry already contains system properties and systemProperties() can be removed in MasterProcessChannelEncoder

Tibor17 requested a review from slawekjaranowski February 24, 2022 11:22

slawekjaranowski requested changes Feb 24, 2022

View reviewed changes

Tibor17 force-pushed the SUREFIRE-2025 branch from 7f6b017 to c33024e Compare February 25, 2022 11:42

slawekjaranowski approved these changes Feb 25, 2022

View reviewed changes

[SUREFIRE-2025] Updated abstractions which helps associating systemPr…

e5e57cf

…operties() with a test context added Javadoc TestSetReportEntry already contains system properties and systemProperties() can be removed in MasterProcessChannelEncoder

Tibor17 force-pushed the SUREFIRE-2025 branch from c33024e to e5e57cf Compare February 25, 2022 13:58

Tibor17 merged commit 64f0fde into master Feb 25, 2022

Tibor17 changed the title ~~[SUREFIRE-2025] Updated abstractions which helps associating systemProperties() with a test~~ [SUREFIRE-2025] Updated abstractions which helps associating systemProperties() with a test context Feb 25, 2022

Tibor17 deleted the SUREFIRE-2025 branch February 25, 2022 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SUREFIRE-2025] Updated abstractions which helps associating systemProperties() with a test context #475

[SUREFIRE-2025] Updated abstractions which helps associating systemProperties() with a test context #475

Tibor17 commented Feb 24, 2022 •

edited

slawekjaranowski Feb 24, 2022

slawekjaranowski Feb 24, 2022

Tibor17 Feb 24, 2022

Tibor17 Feb 24, 2022

slawekjaranowski Feb 24, 2022

slawekjaranowski Feb 24, 2022

Tibor17 Feb 24, 2022 •

edited

Tibor17 Feb 24, 2022 •

edited

slawekjaranowski Feb 24, 2022

Tibor17 Feb 25, 2022

slawekjaranowski Feb 24, 2022

Tibor17 Feb 24, 2022 •

edited

Tibor17 Feb 24, 2022 •

edited

slawekjaranowski Feb 24, 2022

Tibor17 Feb 25, 2022 •

edited

Tibor17 commented Feb 25, 2022 •

edited

		void systemProperties( Map<String, String> sysProps );
		void systemProperties( Map<String, String> sysProps, RunMode runMode, Long testRunId );

		public void systemProperties( Map<String, String> sysProps )
		public void systemProperties( Map<String, String> sysProps, RunMode rm, Long testRunId )

		target.systemProperties( report.getSystemProperties() );
		target.systemProperties( report.getSystemProperties(), null, null );

[SUREFIRE-2025] Updated abstractions which helps associating systemProperties() with a test context #475

[SUREFIRE-2025] Updated abstractions which helps associating systemProperties() with a test context #475

Conversation

Tibor17 commented Feb 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 Feb 24, 2022 • edited

Choose a reason for hiding this comment

Tibor17 Feb 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 Feb 24, 2022 • edited

Choose a reason for hiding this comment

Tibor17 Feb 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 Feb 25, 2022 • edited

Choose a reason for hiding this comment

Tibor17 commented Feb 25, 2022 • edited

Tibor17 commented Feb 24, 2022 •

edited

Tibor17 Feb 24, 2022 •

edited

Tibor17 Feb 24, 2022 •

edited

Tibor17 Feb 24, 2022 •

edited

Tibor17 Feb 24, 2022 •

edited

Tibor17 Feb 25, 2022 •

edited

Tibor17 commented Feb 25, 2022 •

edited