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 2022/02/12 20:42:49 UTC
[GitHub] [maven-surefire] Tibor17 opened a new pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Tibor17 opened a new pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469
The method `void writeTestOutput( String output, boolean newLine, boolean stdout )` appeared in `ForkingRunListener` and `TestSetRunListener` and it was called by `ConsoleOutputCapture`.
The information (`testRunId` and Thread) is associated with the particular run of the test method in the implementation of provider's `RunListener`. So the listener has this information, and not the `ForkingRunListener`.
Due to we will implement enum `RunMode` and `testRunId:long` we should not redirect `void writeTestOutput( String output, boolean newLine, boolean stdout )` to `ForkingRunListener`. Therefore we created the interface `TestRunListener` which is implemented by the provider's listener.
After the previous refactoring of surefire-junit3, we should continue with updating the abstraction in order to complete [SUREFIRE-1860](https://issues.apache.org/jira/browse/SUREFIRE-1860). The changes in SUREFIRE-1860 are big and therefore I would like to split them to an abstraction in this PR, continue with another PRs regarding implementation of encoder/decoder, SimpleReportEntry. It would give us the opportunity to associate the std/out/err logs with test run id (Thread) and deterministically create the reports and this way fix pending issues (junit5, and simplify the listeners in junit4.7 provider). So I am splitting the work in several pieces.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] 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.
- [ ] 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](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.
- [ ] 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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805506131
##########
File path: surefire-providers/common-junit4/src/main/java/org/apache/maven/surefire/common/junit4/JUnit4RunListener.java
##########
@@ -41,12 +43,14 @@
*
*/
public class JUnit4RunListener
- extends org.junit.runner.notification.RunListener
+ extends RunListener
+ implements TestOutputReceiver
{
- protected final RunListener reporter;
+ protected final TestRunListener reporter;
Review comment:
The interface `TestReportListener` contains the wording `report`, pls see the next commit.
--
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
[GitHub] [maven-surefire] Tibor17 merged pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 merged pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806569872
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
@slawekjaranowski
Yesterday I downloaded the latest `maven-idea-codestyle.xml` and used in my IDEA.
Can you pls use the same and reproduce this problem?
Try to press CTRL+ALT+O on this file in e.g. master.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806589916
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
Because this PR has only limited refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output printed during the test without mapping the log message and test Thread. Other refactoring is out of the scope.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806593109
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -255,7 +255,7 @@ public void testReporterFactory()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
This PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806577112
##########
File path: surefire-providers/surefire-junit47/pom.xml
##########
@@ -73,100 +79,6 @@
<propertyName>jacoco.agent</propertyName>
</configuration>
</plugin>
- <plugin>
- <artifactId>maven-dependency-plugin</artifactId>
- <executions>
- <execution>
- <id>main</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.7</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>main-junit47-patch</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- <includes>org/junit/runner/notification/RunListener*</includes>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>test</id>
- <phase>process-test-sources</phase>
- <goals>
- <goal>copy</goal>
- </goals>
- <configuration>
- <outputDirectory>${project.build.directory}/endorsed-test</outputDirectory>
- <overWriteIfNewer>false</overWriteIfNewer>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-assembly-plugin</artifactId>
- <executions>
- <execution>
- <id>patch-junit47</id>
- <phase>process-sources</phase>
- <goals>
- <goal>single</goal>
- </goals>
- <configuration>
- <attach>false</attach>
- <finalName>junit-4.7</finalName>
- <outputDirectory>${project.build.directory}/endorsed</outputDirectory>
- <descriptors>
- <descriptor>src/assembly/assembly.xml</descriptor>
- </descriptors>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-compiler-plugin</artifactId>
- <configuration>
- <compilerArguments>
- <endorseddirs>${project.build.directory}/endorsed</endorseddirs>
- </compilerArguments>
- <testCompilerArguments>
- <endorseddirs>${project.build.directory}/endorsed-test</endorseddirs>
- </testCompilerArguments>
- </configuration>
- </plugin>
<plugin>
Review comment:
Do you know the annotation `ThreadSafe` in JUnit4?
Threadsafe listeners.
This can be applied only on JUnit's `RunListener`.
--
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
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806061530
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
why not use `getTestSetReporter()` instead of `getOrCreateConsoleLogger()` - private method
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -255,7 +255,7 @@ public void testReporterFactory()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
can be mock
--
Surprise - what is foo? 😄 line 245
##########
File path: surefire-providers/surefire-junit47/pom.xml
##########
@@ -73,100 +79,6 @@
<propertyName>jacoco.agent</propertyName>
</configuration>
</plugin>
- <plugin>
- <artifactId>maven-dependency-plugin</artifactId>
- <executions>
- <execution>
- <id>main</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.7</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>main-junit47-patch</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- <includes>org/junit/runner/notification/RunListener*</includes>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>test</id>
- <phase>process-test-sources</phase>
- <goals>
- <goal>copy</goal>
- </goals>
- <configuration>
- <outputDirectory>${project.build.directory}/endorsed-test</outputDirectory>
- <overWriteIfNewer>false</overWriteIfNewer>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-assembly-plugin</artifactId>
- <executions>
- <execution>
- <id>patch-junit47</id>
- <phase>process-sources</phase>
- <goals>
- <goal>single</goal>
- </goals>
- <configuration>
- <attach>false</attach>
- <finalName>junit-4.7</finalName>
- <outputDirectory>${project.build.directory}/endorsed</outputDirectory>
- <descriptors>
- <descriptor>src/assembly/assembly.xml</descriptor>
- </descriptors>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-compiler-plugin</artifactId>
- <configuration>
- <compilerArguments>
- <endorseddirs>${project.build.directory}/endorsed</endorseddirs>
- </compilerArguments>
- <testCompilerArguments>
- <endorseddirs>${project.build.directory}/endorsed-test</endorseddirs>
- </testCompilerArguments>
- </configuration>
- </plugin>
<plugin>
Review comment:
super ... I even didn't try understand it
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -279,7 +279,7 @@ public void testReporterFactoryAware()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
mock
##########
File path: surefire-providers/common-junit4/src/main/java/org/apache/maven/surefire/common/junit4/JUnit4RunListener.java
##########
@@ -58,11 +63,16 @@
*
* @param reporter the reporter to log testing events to
*/
- public JUnit4RunListener( RunListener reporter )
+ public JUnit4RunListener( TestReportListener reporter )
{
this.reporter = reporter;
}
+ public final ConsoleLogger getConsoleLogger()
Review comment:
new public method without Overrides .... some of comments will be useful
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -55,7 +55,7 @@ public void testShouldCreateFactoryWithoutException()
ReporterFactory factory = new ReporterFactory()
Review comment:
can be mock, only reference of object is checked.
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/Surefire746Test.java
##########
@@ -36,26 +36,17 @@
* limitations under the License.
*/
-import java.io.File;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-
+import org.apache.maven.plugin.surefire.log.api.ConsoleLogger;
import org.apache.maven.surefire.api.booter.BaseProviderFactory;
import org.apache.maven.surefire.api.booter.ProviderParameterNames;
-import org.apache.maven.surefire.common.junit4.JUnit4RunListener;
-import org.apache.maven.surefire.common.junit4.Notifier;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
import org.apache.maven.surefire.api.report.ReporterConfiguration;
import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
+import org.apache.maven.surefire.api.report.TestReportListener;
import org.apache.maven.surefire.api.suite.RunResult;
import org.apache.maven.surefire.api.testset.TestSetFailedException;
import org.apache.maven.surefire.api.util.TestsToRun;
-
+import org.apache.maven.surefire.common.junit4.JUnit4RunListener;
+import org.apache.maven.surefire.common.junit4.Notifier;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
Review comment:
invalid import order
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
invalid import order
https://maven.apache.org/developers/conventions/code.html#java-code-convention-import-layouts
--
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
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806061530
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
why not use `getTestSetReporter()` instead of `getOrCreateConsoleLogger()` - private method
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -255,7 +255,7 @@ public void testReporterFactory()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
can be mock
--
Surprise - what is foo? 😄 line 245
##########
File path: surefire-providers/surefire-junit47/pom.xml
##########
@@ -73,100 +79,6 @@
<propertyName>jacoco.agent</propertyName>
</configuration>
</plugin>
- <plugin>
- <artifactId>maven-dependency-plugin</artifactId>
- <executions>
- <execution>
- <id>main</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.7</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>main-junit47-patch</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- <includes>org/junit/runner/notification/RunListener*</includes>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>test</id>
- <phase>process-test-sources</phase>
- <goals>
- <goal>copy</goal>
- </goals>
- <configuration>
- <outputDirectory>${project.build.directory}/endorsed-test</outputDirectory>
- <overWriteIfNewer>false</overWriteIfNewer>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-assembly-plugin</artifactId>
- <executions>
- <execution>
- <id>patch-junit47</id>
- <phase>process-sources</phase>
- <goals>
- <goal>single</goal>
- </goals>
- <configuration>
- <attach>false</attach>
- <finalName>junit-4.7</finalName>
- <outputDirectory>${project.build.directory}/endorsed</outputDirectory>
- <descriptors>
- <descriptor>src/assembly/assembly.xml</descriptor>
- </descriptors>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-compiler-plugin</artifactId>
- <configuration>
- <compilerArguments>
- <endorseddirs>${project.build.directory}/endorsed</endorseddirs>
- </compilerArguments>
- <testCompilerArguments>
- <endorseddirs>${project.build.directory}/endorsed-test</endorseddirs>
- </testCompilerArguments>
- </configuration>
- </plugin>
<plugin>
Review comment:
super ... I even didn't try understand it
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -279,7 +279,7 @@ public void testReporterFactoryAware()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
mock
##########
File path: surefire-providers/common-junit4/src/main/java/org/apache/maven/surefire/common/junit4/JUnit4RunListener.java
##########
@@ -58,11 +63,16 @@
*
* @param reporter the reporter to log testing events to
*/
- public JUnit4RunListener( RunListener reporter )
+ public JUnit4RunListener( TestReportListener reporter )
{
this.reporter = reporter;
}
+ public final ConsoleLogger getConsoleLogger()
Review comment:
new public method without Overrides .... some of comments will be useful
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -55,7 +55,7 @@ public void testShouldCreateFactoryWithoutException()
ReporterFactory factory = new ReporterFactory()
Review comment:
can be mock, only reference of object is checked.
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/Surefire746Test.java
##########
@@ -36,26 +36,17 @@
* limitations under the License.
*/
-import java.io.File;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-
+import org.apache.maven.plugin.surefire.log.api.ConsoleLogger;
import org.apache.maven.surefire.api.booter.BaseProviderFactory;
import org.apache.maven.surefire.api.booter.ProviderParameterNames;
-import org.apache.maven.surefire.common.junit4.JUnit4RunListener;
-import org.apache.maven.surefire.common.junit4.Notifier;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
import org.apache.maven.surefire.api.report.ReporterConfiguration;
import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
+import org.apache.maven.surefire.api.report.TestReportListener;
import org.apache.maven.surefire.api.suite.RunResult;
import org.apache.maven.surefire.api.testset.TestSetFailedException;
import org.apache.maven.surefire.api.util.TestsToRun;
-
+import org.apache.maven.surefire.common.junit4.JUnit4RunListener;
+import org.apache.maven.surefire.common.junit4.Notifier;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
Review comment:
invalid import order
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
invalid import order
https://maven.apache.org/developers/conventions/code.html#java-code-convention-import-layouts
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#issuecomment-1038880742
@slawekjaranowski
Let's have a look. That's all from my side.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806589916
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output printed during the test without mapping the log message and test Thread. Other refactoring is out of the scope.
--
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
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#issuecomment-1038574801
@slawekjaranowski
I have made one more commit because both listeners (ForkingRunListener and TestSetRunListener) are a mirror of the same interface in fork/plugin JVM and so they should implement one and the same interface as mandatory. This way we have avoided ugly casting to type. Any combinations of two interfaces out of three (RunListener, TestOutputReceiver, ConsoleLogger) are avoided and the only TestReportListener is used. Removed ConsoleStream interface and used ConsoleLogger instead. Simplified code around logger in JUnitCoreProvider. Renamed method in ReporterFactory.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805531910
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
The data flow `fork` -> `plugin` is all about reporting events.
The data flow `plugin` -> `fork` is about commands.
So this is Command Event based architecture. Similar wordings are in the enterprise world CQRS and Event Sourcing.
The first data flow usually sends objects in the form of:
- new XxxEvent(new ReportEntry()) - typically test success
- new XxxEvent(<string>) - typically std/out from a Thread of a running test
- new XxxEvent(<string>) - typically console error, debug
Therefore there is such a wording `report` because the provider (InPlugin or fork) sends a report back to the plugin, and the provider is under the plugin in the hierarchy because the plugin is a manager of providers.
Now it is visible in the inheritance
`ForkingRunListener` -> `RunListener`, `TestOutputReceiver`, `ConsoleLogger`.
These are 3 types and three interfaces.
I know that you have a problem with the name of class `ForkingRunListener` but it is not mandatory for the developers because the developers and their code operates with abstractions and their code instantiates the object somewhere but the interfaces are use everywhere. The interfaces have different purpose because we do not want `ParallelComputer` to send TEST_SUCCESS of course not, and the PC sends console internal error if any. This is called information hiding. **So there are reports, all these three interfaces are about reports but different group of reports.**
Regarding `ForkingRunListener`, whatsoever do you like `ForkingReportListener` more?
From the history, you should know how the code was before. These methods of reports were everywhere. They did not have one place like it is not. Their implementation was really everywhere, the encoder did not exist in one place and so it was very hard to maintain the code. Naturally this situation happened because we are OSS, and in the OSS you start from simple use cases and you do not have time to think of encapsulation, information hiding, and such things.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805221122
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Yes of course becasue the console logger is used to report errors from a forked surefire JVM back to the plugin JVM in cases when internal code throws unexpected exception (not the 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805221122
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Yes of course becasue the console logger is used to report errors from JVM back to plugin JVM in cases when internal code throws unexpected exception (not the tests).
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Yes of course becasue the console logger is used to report errors from a forked surefire JVM back to plugin JVM in cases when internal code throws unexpected exception (not the tests).
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Yes of course becasue the console logger is used to report errors from a forked surefire JVM back to the plugin JVM in cases when internal code throws unexpected exception (not the tests).
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Yes of course becasue the console logger is used to report internal errors from a forked surefire JVM back to the plugin JVM in cases when internal code throws unexpected exception (not the tests).
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/ReporterFactory.java
##########
@@ -33,7 +33,7 @@
*
* @return A reporter instance
*/
- RunListener createReporter();
+ TestRunListener createReporter();
Review comment:
In this architecture we have two sides (forked JVM, plugin JVM) having implemented the same interface RunListener:
1. side is ForkingRunListener in the fork JVM
2. TestSetRunListener in the plugin JVM
The serialization between these two JVM ensures that the ReportEntries are transfered from the fork to the plugin.
This is the whole idea behind them.
We can see these renaming issues from the another angle, and so that there is a reporter. Listener means that our surefire `ForkingRunListener` is a delegate within JUnit's RunListeners in the fork. On the other side, the TestSetRunListener is a listener registered in the `ForkClient`.
We can think about it a bit deeper and we will find the best names for these interfaces and methods.
Anyway, we will remove deprecated code, we will create Builders for constructors with too many parameters, so we will have a chance to refactor a lot.
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/ReporterFactory.java
##########
@@ -33,7 +33,7 @@
*
* @return A reporter instance
*/
- RunListener createReporter();
+ TestRunListener createReporter();
Review comment:
In this architecture we have two sides (forked JVM, plugin JVM) having implemented the same interface RunListener:
1. side is ForkingRunListener in the fork JVM
2. TestSetRunListener in the plugin JVM
The serialization between these two JVM ensures that the ReportEntries are transfered from the fork to the plugin.
This is the whole idea behind them.
We can see these renaming issues from the another angle, and so that there is a reporter. Listener means that our surefire `ForkingRunListener` is a delegate within JUnit's RunListeners in the fork. On the other side, the `TestSetRunListener` is a listener registered in the `ForkClient` which is part of plugin JVM.
We can think about it a bit deeper and we will find the best names for these interfaces and methods.
Anyway, we will remove deprecated code, we will create Builders for constructors with too many parameters, so we will have a chance to refactor a lot.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805568101
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
@slawekjaranowski
This line of code `logger = (ConsoleLogger) forkingReporterFactory.createReporter();` does not exist in the second commit. The line is `logger = forkingReporterFactory.createTestReportListener();`.
Regarding the `ConsoleLoggerDecorator`, the decorator is used in the InPlugin execution, not in fork. See the `CommonReflector#createConsoleLogger()`.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805531910
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
The data flow `fork` -> `plugin` is all about reporting events.
The data flow `plugin` -> `fork` is about commands.
So this is Command Event based architecture. Similar wordings are in the enterprise world CQRS and Event Sourcing.
The first data flow usually sends objects in the form of:
- new XxxEvent(new ReportEntry()) - typically test success
- new XxxEvent(<string>) - typically std/out from a Thread of a running test
- new XxxEvent(<string>) - typically console error, debug
Therefore there is such a wording `report` because the provider (InPlugin or fork) sends a report back to the plugin, and the provider is under the plugin in the hierarchy because the plugin is a manager of providers.
Now it is visible in the inheritance
`ForkingRunListener` -> `RunListener`, `TestOutputReceiver`, `ConsoleLogger`.
These are 3 types and three interfaces.
I know that you have a problem with the name of class `ForkingRunListener` but it is not mandatory for the developers because the developers and their code operates with abstractions and their code instantiates the object somewhere but the interfaces are used everywhere. The interfaces have different purpose because we do not want `ParallelComputer` to send TEST_SUCCESS of course not, and the PC sends console internal error if any. This is called information hiding. **So there are reports, all these three interfaces are about reports but different group of reports.**
Regarding `ForkingRunListener`, whatsoever, do you like `ForkingReportListener` more?
From the history, you should know how the code was before. These methods of reports were everywhere. They did not exist one place like it is now in one hotspot. Their implementation was really everywhere, the encoder did not exist in one place and so the encoder's logic was everywhere and it was very hard to maintain the code. Naturally, this situation happened because we are OSS, and in the OSS you start from simple use cases and you do not have time to think of encapsulation, information hiding, and such things like a cost of maintenance, and such style results in paches and not in real fixes.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805222109
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/ReporterFactory.java
##########
@@ -33,7 +33,7 @@
*
* @return A reporter instance
*/
- RunListener createReporter();
+ TestRunListener createReporter();
Review comment:
In this architecture we have two sides (forked JVM, plugin JVM) having implemented the same interface RunListener:
1. side is ForkingRunListener in the fork JVM
2. TestSetRunListener in the plugin JVM
The serialization between these two JVM ensures that the ReportEntries are transfered from the fork to the plugin.
This is the whole idea behind them.
We can see these renaming issues from the another angle, and so that there is a reporter. Listener means that our surefire `ForkingRunListener` is a delegate within JUnit's RunListeners in the fork. On the other side, the TestSetRunListener is a listener registered in the `ForkClient`.
We can think about it a bit deeper and we will find the best names for these interfaces and methods.
Anyway, we will remove deprecated code, we will create Builders for constructors with too many parameters, so we will have a chance to refactor a lot.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805567287
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/ReporterFactory.java
##########
@@ -33,7 +33,7 @@
*
* @return A reporter instance
*/
- RunListener createReporter();
+ TestRunListener createReporter();
Review comment:
@slawekjaranowski yes, the method was renamed in the second commit.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806592706
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -55,7 +55,7 @@ public void testShouldCreateFactoryWithoutException()
ReporterFactory factory = new ReporterFactory()
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806593507
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -279,7 +279,7 @@ public void testReporterFactoryAware()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
@slawekjaranowski This PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#issuecomment-1038574801
@slawekjaranowski
I have made one more commit because both listeners (ForkingRunListener and TestSetRunListener) are a mirror of the same fork/plugin JVM and so they should implement one and the same interface as mandatory. This way we have avoided ugly casting to type. Any combinations of two interfaces out of three (RunListener, TestOutputReceiver, ConsoleLogger) are avoided and the only TestReportListener is used. Removed ConsoleStream interface and used ConsoleLogger instead. Simplified code around logger in JUnitCoreProvider. Renamed method in ReporterFactory.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805531910
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
The data flow `fork` -> `plugin` is all about reporting events.
The data flow `plugin` -> `fork` is about commands.
So this is Command Event based architecture. Similar wordings are in the enterprise world CQRS and Event Sourcing.
The first data flow usually sends objects in the form of:
- new XxxEvent(new ReportEntry()) - typically test success
- new XxxEvent(<string>) - typically std/out from a Thread of a running test
- new XxxEvent(<string>) - typically console error, debug
Therefore there is such a wording `report` because the provider (InPlugin or fork) sends a report back to the plugin, and the provider is under the plugin in the hierarchy because the plugin is a manager of providers.
Now it is visible in the inheritance
`ForkingRunListener` -> `RunListener`, `TestOutputReceiver`, `ConsoleLogger`.
These are 3 types and three interfaces.
I know that you have a problem with the name of class `ForkingRunListener` but it is not mandatory for the developers because the developers and their code operates with abstractions and their code instantiates the object somewhere but the interfaces are used everywhere. The interfaces have different purpose because we do not want `ParallelComputer` to send TEST_SUCCESS of course not, and the PC sends console internal error if any. This is called information hiding. **So there are reports, all these three interfaces are about reports but different group of reports.**
Regarding `ForkingRunListener`, whatsoever, do you like `ForkingReportListener` more?
From the history, you should know how the code was before. These methods of reports were everywhere. They did not exist one place like it is now in one hotspot. Their implementation was really everywhere, the encoder did not exist in one place and so the encoder's logic was everywhere and it was very hard to maintain the code. Naturally, this situation happened because we are OSS, and in the OSS you start from simple use cases and you do not have time to think of encapsulation, information hiding, and such things cost of maintenance, and such style results in paches and not in real fixes.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805222109
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/ReporterFactory.java
##########
@@ -33,7 +33,7 @@
*
* @return A reporter instance
*/
- RunListener createReporter();
+ TestRunListener createReporter();
Review comment:
In this architecture we have two sides (forked JVM, plugin JVM) having implemented the same interface RunListener:
1. side is ForkingRunListener in the fork JVM
2. TestSetRunListener in the plugin JVM
The serialization between these two JVM ensures that the ReportEntries are transfered from the fork to the plugin.
This is the whole idea behind them.
We can see these renaming issues from the another angle, and so that there is a reporter. Listener means that our surefire `ForkingRunListener` is a delegate within JUnit's RunListeners in the fork. On the other side, the `TestSetRunListener` is a listener registered in the `ForkClient` which is part of plugin JVM.
We can think about it a bit deeper and we will find the best names for these interfaces and methods.
Anyway, we will remove deprecated code, we will create Builders for constructors with too many parameters, so we will have a chance to refactor a lot.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805531910
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
The data flow `fork` -> `plugin` is all about reporting events.
The data flow `plugin` -> `fork` is about commands.
So this is Command Event based architecture. Similar wordings are in the enterprise world CQRS and Event Sourcing.
The first data flow usually sends objects in the form of:
- new XxxEvent(new ReportEntry()) - typically test success
- new XxxEvent(<string>) - typically std/out from a Thread of a running test
- new XxxEvent(<string>) - typically console error, debug
Therefore there is such a wording `report` because the provider (InPlugin or fork) sends a report back to the plugin, and the provider is under the plugin in the hierarchy because the plugin is a manager of providers.
Now it is visible in the inheritance
`ForkingRunListener` -> `RunListener`, `TestOutputReceiver`, `ConsoleLogger`.
These are 3 types and three interfaces.
I know that you have a problem with the name of class `ForkingRunListener` but it is not mandatory for the developers because the developers and their code operates with abstractions and their code instantiates the object somewhere but the interfaces are used everywhere. The interfaces have different purpose because we do not want `ParallelComputer` to send TEST_SUCCESS of course not, and the PC sends console internal error if any. This is called information hiding. **So there are reports, all these three interfaces are about reports but different group of reports.**
Regarding `ForkingRunListener`, whatsoever, do you like `ForkingReportListener` more?
From the history, you should know how the code was before. These methods of reports were everywhere. They did not exist one place like it is now in one hotspot. Their implementation was really everywhere, the encoder did not exist in one place and so the encoder's logic was everywhere and it was very hard to maintain the code. Naturally, this situation happened because we are OSS, and in the OSS you start from simple use cases and you do not have time to think of encapsulation, information hiding, and such things cost of maintenance, and such style results in paches and but nit in real fixes.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805222109
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/ReporterFactory.java
##########
@@ -33,7 +33,7 @@
*
* @return A reporter instance
*/
- RunListener createReporter();
+ TestRunListener createReporter();
Review comment:
In this architecture we have two sides (forked JVM, plugin JVM) having implemented the same interface RunListener:
1. side is ForkingRunListener in the fork JVM
2. TestSetRunListener in the plugin JVM
The serialization between these two JVM ensures that the ReportEntries are transfered from the fork to the plugin.
This is the whole idea behind them.
We can see these renaming issues from the another angle, and so that there is a reporter. Listener means that our surefire `ForkingRunListener` is a delegate within JUnit's RunListeners in the fork. On the other side, the `TestSetRunListener` is a listener registered in the `ForkClient` which is part of plugin JVM.
We can think about it a bit deeper and we will find the best names for these interfaces and methods.
Anyway, we will remove deprecated code, we will create Builders for constructors with too many parameters, so we will have a chance to refactor a lot.
--
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
[GitHub] [maven-surefire] Tibor17 removed a comment on pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 removed a comment on pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#issuecomment-1038880742
@slawekjaranowski
Let's have a look. That's all from my side.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806606750
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
fixed
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/Surefire746Test.java
##########
@@ -36,26 +36,17 @@
* limitations under the License.
*/
-import java.io.File;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-
+import org.apache.maven.plugin.surefire.log.api.ConsoleLogger;
import org.apache.maven.surefire.api.booter.BaseProviderFactory;
import org.apache.maven.surefire.api.booter.ProviderParameterNames;
-import org.apache.maven.surefire.common.junit4.JUnit4RunListener;
-import org.apache.maven.surefire.common.junit4.Notifier;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
import org.apache.maven.surefire.api.report.ReporterConfiguration;
import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
+import org.apache.maven.surefire.api.report.TestReportListener;
import org.apache.maven.surefire.api.suite.RunResult;
import org.apache.maven.surefire.api.testset.TestSetFailedException;
import org.apache.maven.surefire.api.util.TestsToRun;
-
+import org.apache.maven.surefire.common.junit4.JUnit4RunListener;
+import org.apache.maven.surefire.common.junit4.Notifier;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
Review comment:
fixed
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805221122
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Yes of course becasue the console logger is used to report internal errors from a forked surefire JVM back to the plugin JVM in cases when internal code throws unexpected exception (not the 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805506547
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/TestRunListener.java
##########
@@ -0,0 +1,28 @@
+package org.apache.maven.surefire.api.report;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ *
+ */
+public interface TestRunListener
+ extends RunListener, TestOutputReceiver
+{
+}
Review comment:
done
--
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
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805308947
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Ok, I can wrong
On fork side
`ForkingRunListener` is used as ConsoleLogger
instance of `ForkingRunListener` is created by `ForkingReporterFactory#createReporter`
in `ForkedBooter` I see
```
forkingReporterFactory = createForkingReporterFactory();
logger = (ConsoleLogger) forkingReporterFactory.createReporter();
```
but when I see `ConsoleLoggerDecorator` - everything can happen in runtime
so don't spend more time on this ...
maybe after more time it will be more clear for me
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805568101
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
@slawekjaranowski
This line of code `logger = (ConsoleLogger) forkingReporterFactory.createReporter();` does not exist in the second commit. The line is `logger = forkingReporterFactory.createTestReportListener();`.
Regarding the `ConsoleLoggerDecorator`, the decorator is used in the InPlugin execution, not in fork. See the `CommonReflector#createConsoleLogger()`. Then see `Object factory = surefireReflector.createReportingReporterFactory( startupReportConfig, consoleLogger );` in the class `InPluginVMSurefireStarter`. The decorator is used in `ForkStarter#getSuitesIterator()` as well.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805568101
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
@slawekjaranowski
This line of code `logger = (ConsoleLogger) forkingReporterFactory.createReporter();` does not exist in the second commit. The line is `logger = forkingReporterFactory.createTestReportListener();`.
Regarding the `ConsoleLoggerDecorator`, the decorator is used in the InPlugin execution, not in fork. See the `CommonReflector#createConsoleLogger()`. Then see `Object factory = surefireReflector.createReportingReporterFactory( startupReportConfig, consoleLogger );` in the class `InPluginVMSurefireStarter`.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806569872
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
@slawekjaranowski
Yesterday I downloaded the latest `maven-idea-codestyle.xml` and used in my IDEA.
Can you pls use the same and reproduce this problem?
Try to press CTRL+ALT+O on this file in e.g. master.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805608658
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
@slawekjaranowski
If you mean to use `ConsoleLoggerDecorator` in order to cast `Object` to `ConsoleLogger`, then I have to say that it is too heavy procedure. Now the second commit does not use casting types because we have a composition of interfaces, so we return the subinterface.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806577112
##########
File path: surefire-providers/surefire-junit47/pom.xml
##########
@@ -73,100 +79,6 @@
<propertyName>jacoco.agent</propertyName>
</configuration>
</plugin>
- <plugin>
- <artifactId>maven-dependency-plugin</artifactId>
- <executions>
- <execution>
- <id>main</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.7</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>main-junit47-patch</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- <includes>org/junit/runner/notification/RunListener*</includes>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>test</id>
- <phase>process-test-sources</phase>
- <goals>
- <goal>copy</goal>
- </goals>
- <configuration>
- <outputDirectory>${project.build.directory}/endorsed-test</outputDirectory>
- <overWriteIfNewer>false</overWriteIfNewer>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-assembly-plugin</artifactId>
- <executions>
- <execution>
- <id>patch-junit47</id>
- <phase>process-sources</phase>
- <goals>
- <goal>single</goal>
- </goals>
- <configuration>
- <attach>false</attach>
- <finalName>junit-4.7</finalName>
- <outputDirectory>${project.build.directory}/endorsed</outputDirectory>
- <descriptors>
- <descriptor>src/assembly/assembly.xml</descriptor>
- </descriptors>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-compiler-plugin</artifactId>
- <configuration>
- <compilerArguments>
- <endorseddirs>${project.build.directory}/endorsed</endorseddirs>
- </compilerArguments>
- <testCompilerArguments>
- <endorseddirs>${project.build.directory}/endorsed-test</endorseddirs>
- </testCompilerArguments>
- </configuration>
- </plugin>
<plugin>
Review comment:
Do you know the annotation `ThreadSafe` in JUnit4?
Threadsafe listeners.
This can be applied only on JUnit's `RunListener`. If it is done, the JUnit's synchronization is avoided.
This annotation was used on the top of our `org.apache` listeners and so this improvement was deleted.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806569872
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
@slawekjaranowski
Yesterday I downloaded the latest `maven-idea-codestyle.xml` and used in my IDEA.
Can you pls use the same and reproduce this problem?
Try to press CTRL+ALT+O on this file in e.g. master.
##########
File path: surefire-providers/surefire-junit47/pom.xml
##########
@@ -73,100 +79,6 @@
<propertyName>jacoco.agent</propertyName>
</configuration>
</plugin>
- <plugin>
- <artifactId>maven-dependency-plugin</artifactId>
- <executions>
- <execution>
- <id>main</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.7</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>main-junit47-patch</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- <includes>org/junit/runner/notification/RunListener*</includes>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>test</id>
- <phase>process-test-sources</phase>
- <goals>
- <goal>copy</goal>
- </goals>
- <configuration>
- <outputDirectory>${project.build.directory}/endorsed-test</outputDirectory>
- <overWriteIfNewer>false</overWriteIfNewer>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-assembly-plugin</artifactId>
- <executions>
- <execution>
- <id>patch-junit47</id>
- <phase>process-sources</phase>
- <goals>
- <goal>single</goal>
- </goals>
- <configuration>
- <attach>false</attach>
- <finalName>junit-4.7</finalName>
- <outputDirectory>${project.build.directory}/endorsed</outputDirectory>
- <descriptors>
- <descriptor>src/assembly/assembly.xml</descriptor>
- </descriptors>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-compiler-plugin</artifactId>
- <configuration>
- <compilerArguments>
- <endorseddirs>${project.build.directory}/endorsed</endorseddirs>
- </compilerArguments>
- <testCompilerArguments>
- <endorseddirs>${project.build.directory}/endorsed-test</endorseddirs>
- </testCompilerArguments>
- </configuration>
- </plugin>
<plugin>
Review comment:
Do you know the annotation `ThreadSafe` in JUnit4?
Threadsafe listeners.
This can be applied only on JUnit's `RunListener`.
##########
File path: surefire-providers/surefire-junit47/pom.xml
##########
@@ -73,100 +79,6 @@
<propertyName>jacoco.agent</propertyName>
</configuration>
</plugin>
- <plugin>
- <artifactId>maven-dependency-plugin</artifactId>
- <executions>
- <execution>
- <id>main</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.7</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>main-junit47-patch</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- <includes>org/junit/runner/notification/RunListener*</includes>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>test</id>
- <phase>process-test-sources</phase>
- <goals>
- <goal>copy</goal>
- </goals>
- <configuration>
- <outputDirectory>${project.build.directory}/endorsed-test</outputDirectory>
- <overWriteIfNewer>false</overWriteIfNewer>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-assembly-plugin</artifactId>
- <executions>
- <execution>
- <id>patch-junit47</id>
- <phase>process-sources</phase>
- <goals>
- <goal>single</goal>
- </goals>
- <configuration>
- <attach>false</attach>
- <finalName>junit-4.7</finalName>
- <outputDirectory>${project.build.directory}/endorsed</outputDirectory>
- <descriptors>
- <descriptor>src/assembly/assembly.xml</descriptor>
- </descriptors>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-compiler-plugin</artifactId>
- <configuration>
- <compilerArguments>
- <endorseddirs>${project.build.directory}/endorsed</endorseddirs>
- </compilerArguments>
- <testCompilerArguments>
- <endorseddirs>${project.build.directory}/endorsed-test</endorseddirs>
- </testCompilerArguments>
- </configuration>
- </plugin>
<plugin>
Review comment:
Do you know the annotation `ThreadSafe` in JUnit4?
Threadsafe listeners.
This can be applied only on JUnit's `RunListener`. If it is done, the JUnit's synchronization is avoided.
This annotation was used on the top of our `org.apache` listeners and so this improvement was deleted.
##########
File path: surefire-providers/surefire-junit47/pom.xml
##########
@@ -73,100 +79,6 @@
<propertyName>jacoco.agent</propertyName>
</configuration>
</plugin>
- <plugin>
- <artifactId>maven-dependency-plugin</artifactId>
- <executions>
- <execution>
- <id>main</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.7</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>main-junit47-patch</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- <includes>org/junit/runner/notification/RunListener*</includes>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>test</id>
- <phase>process-test-sources</phase>
- <goals>
- <goal>copy</goal>
- </goals>
- <configuration>
- <outputDirectory>${project.build.directory}/endorsed-test</outputDirectory>
- <overWriteIfNewer>false</overWriteIfNewer>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-assembly-plugin</artifactId>
- <executions>
- <execution>
- <id>patch-junit47</id>
- <phase>process-sources</phase>
- <goals>
- <goal>single</goal>
- </goals>
- <configuration>
- <attach>false</attach>
- <finalName>junit-4.7</finalName>
- <outputDirectory>${project.build.directory}/endorsed</outputDirectory>
- <descriptors>
- <descriptor>src/assembly/assembly.xml</descriptor>
- </descriptors>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-compiler-plugin</artifactId>
- <configuration>
- <compilerArguments>
- <endorseddirs>${project.build.directory}/endorsed</endorseddirs>
- </compilerArguments>
- <testCompilerArguments>
- <endorseddirs>${project.build.directory}/endorsed-test</endorseddirs>
- </testCompilerArguments>
- </configuration>
- </plugin>
<plugin>
Review comment:
Do you know the annotation `ThreadSafe` in JUnit4?
Threadsafe listeners.
This can be applied only on JUnit's `RunListener`. If it is done, the JUnit's synchronization is avoided.
This annotation was used on the top of our `org.apache` listeners and so this improvement was deleted.
I am talking about the super types of the listeners.
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
Because this PR has only limited refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output printed during the test without mapping the log message and test Thread. Other refactoring is out of the scope.
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output printed during the test without mapping the log message and test Thread. Other refactoring is out of the scope.
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output printed with the Thread of the test without refactoring of these interfaces.
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -55,7 +55,7 @@ public void testShouldCreateFactoryWithoutException()
ReporterFactory factory = new ReporterFactory()
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -255,7 +255,7 @@ public void testReporterFactory()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -279,7 +279,7 @@ public void testReporterFactoryAware()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
@slawekjaranowski This PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
##########
File path: surefire-providers/common-junit4/src/main/java/org/apache/maven/surefire/common/junit4/JUnit4RunListener.java
##########
@@ -58,11 +63,16 @@
*
* @param reporter the reporter to log testing events to
*/
- public JUnit4RunListener( RunListener reporter )
+ public JUnit4RunListener( TestReportListener reporter )
{
this.reporter = reporter;
}
+ public final ConsoleLogger getConsoleLogger()
Review comment:
There cannot be Override because there is no such super type having a method to override.
This class in another module, so it must be public and of course without Overide - no abstract method in super type.
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
@slawekjaranowski
Yesterday I downloaded the latest `maven-idea-codestyle.xml` and used in my IDEA.
Can you pls use the same and reproduce this problem?
Try to press CTRL+ALT+O on this file in e.g. master.
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
As it seems I used another checkstyle profile in IDEA or I used Maven profile with old content.
I will make a new commit for this.
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -255,7 +255,7 @@ public void testReporterFactory()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
This PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -55,7 +55,7 @@ public void testShouldCreateFactoryWithoutException()
ReporterFactory factory = new ReporterFactory()
Review comment:
This PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
fixed
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/Surefire746Test.java
##########
@@ -36,26 +36,17 @@
* limitations under the License.
*/
-import java.io.File;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-
+import org.apache.maven.plugin.surefire.log.api.ConsoleLogger;
import org.apache.maven.surefire.api.booter.BaseProviderFactory;
import org.apache.maven.surefire.api.booter.ProviderParameterNames;
-import org.apache.maven.surefire.common.junit4.JUnit4RunListener;
-import org.apache.maven.surefire.common.junit4.Notifier;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
import org.apache.maven.surefire.api.report.ReporterConfiguration;
import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
+import org.apache.maven.surefire.api.report.TestReportListener;
import org.apache.maven.surefire.api.suite.RunResult;
import org.apache.maven.surefire.api.testset.TestSetFailedException;
import org.apache.maven.surefire.api.util.TestsToRun;
-
+import org.apache.maven.surefire.common.junit4.JUnit4RunListener;
+import org.apache.maven.surefire.common.junit4.Notifier;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
Review comment:
fixed
--
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
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805212066
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/ReporterFactory.java
##########
@@ -33,7 +33,7 @@
*
* @return A reporter instance
*/
- RunListener createReporter();
+ TestRunListener createReporter();
Review comment:
rename method `createReporter` -> `crateTestRunListener` WDYT?
javadoc for this method is still actual?
##########
File path: surefire-providers/common-junit4/src/main/java/org/apache/maven/surefire/common/junit4/JUnit4RunListener.java
##########
@@ -41,12 +43,14 @@
*
*/
public class JUnit4RunListener
- extends org.junit.runner.notification.RunListener
+ extends RunListener
+ implements TestOutputReceiver
{
- protected final RunListener reporter;
+ protected final TestRunListener reporter;
Review comment:
maybe also change reported -> testRunListener
and in other similar place
##########
File path: surefire-providers/surefire-junit47/src/main/java/org/apache/maven/surefire/junitcore/ConcurrentRunListener.java
##########
@@ -61,10 +60,10 @@
this.reportImmediately = reportImmediately;
this.classMethodCounts = classMethodCounts;
this.consoleStream = consoleStream;
- reporterManagerThreadLocal = new ThreadLocal<RunListener>()
+ reporterManagerThreadLocal = new ThreadLocal<TestRunListener>()
{
@Override
- protected RunListener initialValue()
+ protected TestRunListener initialValue()
{
return reporterFactory.createReporter();
}
Review comment:
now can be like:
```
reporterManagerThreadLocal = ThreadLocal.withInitial( reporterFactory::createReporter );
```
TestSetFailedException is not thrown from this constructor - line 58
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
still we need implements `ConsoleLogger`?
class `TestSetRunListener` is created in one place in `DefaultReporterFactory#createReporter` as `TestRunListener` ...
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/TestRunListener.java
##########
@@ -0,0 +1,28 @@
+package org.apache.maven.surefire.api.report;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ *
+ */
+public interface TestRunListener
+ extends RunListener, TestOutputReceiver
+{
+}
Review comment:
New interface - it will be good to add javadoc with description what purpose of this interface is.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805221122
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Yes of course becasue the console logger is used to report errors from a forked surefire JVM back to plugin JVM in cases when internal code throws unexpected exception (not the 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806577112
##########
File path: surefire-providers/surefire-junit47/pom.xml
##########
@@ -73,100 +79,6 @@
<propertyName>jacoco.agent</propertyName>
</configuration>
</plugin>
- <plugin>
- <artifactId>maven-dependency-plugin</artifactId>
- <executions>
- <execution>
- <id>main</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.7</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>main-junit47-patch</id>
- <phase>process-sources</phase>
- <goals>
- <goal>unpack</goal>
- </goals>
- <configuration>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- <overWrite>true</overWrite>
- <outputDirectory>${project.build.directory}/endorsed-tmp</outputDirectory>
- <includes>org/junit/runner/notification/RunListener*</includes>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- <execution>
- <id>test</id>
- <phase>process-test-sources</phase>
- <goals>
- <goal>copy</goal>
- </goals>
- <configuration>
- <outputDirectory>${project.build.directory}/endorsed-test</outputDirectory>
- <overWriteIfNewer>false</overWriteIfNewer>
- <artifactItems>
- <artifactItem>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <version>4.12</version>
- <type>jar</type>
- </artifactItem>
- </artifactItems>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-assembly-plugin</artifactId>
- <executions>
- <execution>
- <id>patch-junit47</id>
- <phase>process-sources</phase>
- <goals>
- <goal>single</goal>
- </goals>
- <configuration>
- <attach>false</attach>
- <finalName>junit-4.7</finalName>
- <outputDirectory>${project.build.directory}/endorsed</outputDirectory>
- <descriptors>
- <descriptor>src/assembly/assembly.xml</descriptor>
- </descriptors>
- </configuration>
- </execution>
- </executions>
- </plugin>
- <plugin>
- <artifactId>maven-compiler-plugin</artifactId>
- <configuration>
- <compilerArguments>
- <endorseddirs>${project.build.directory}/endorsed</endorseddirs>
- </compilerArguments>
- <testCompilerArguments>
- <endorseddirs>${project.build.directory}/endorsed-test</endorseddirs>
- </testCompilerArguments>
- </configuration>
- </plugin>
<plugin>
Review comment:
Do you know the annotation `ThreadSafe` in JUnit4?
Threadsafe listeners.
This can be applied only on JUnit's `RunListener`. If it is done, the JUnit's synchronization is avoided.
This annotation was used on the top of our `org.apache` listeners and so this improvement was deleted.
I am talking about the super types of the listeners.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806596804
##########
File path: surefire-providers/common-junit4/src/main/java/org/apache/maven/surefire/common/junit4/JUnit4RunListener.java
##########
@@ -58,11 +63,16 @@
*
* @param reporter the reporter to log testing events to
*/
- public JUnit4RunListener( RunListener reporter )
+ public JUnit4RunListener( TestReportListener reporter )
{
this.reporter = reporter;
}
+ public final ConsoleLogger getConsoleLogger()
Review comment:
There cannot be Override because there is no such super type having a method to override.
This class in another module, so it must be public and of course without Overide - no abstract method in super type.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806589916
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output printed with the Thread of the test without refactoring of these interfaces.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806593109
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -255,7 +255,7 @@ public void testReporterFactory()
ReporterFactory reporterFactory = new ReporterFactory()
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806598361
##########
File path: surefire-providers/surefire-junit47/src/test/java/org/apache/maven/surefire/junitcore/ConcurrentRunListenerTest.java
##########
@@ -19,26 +19,24 @@
* under the License.
*/
-import java.io.ByteArrayOutputStream;
-import java.io.PrintStream;
-import java.util.HashMap;
-import java.util.Map;
-import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
-import org.apache.maven.surefire.api.report.ConsoleStream;
-import org.apache.maven.surefire.api.report.DefaultDirectConsoleReporter;
-import org.apache.maven.surefire.api.report.ReporterFactory;
-import org.apache.maven.surefire.api.report.RunListener;
-import org.apache.maven.surefire.report.RunStatistics;
-import org.apache.maven.surefire.api.testset.TestSetFailedException;
-
import junit.framework.Assert;
import junit.framework.TestCase;
import junit.framework.TestSuite;
+import org.apache.maven.plugin.surefire.report.DefaultReporterFactory;
+import org.apache.maven.surefire.api.report.ReporterFactory;
+import org.apache.maven.surefire.api.report.TestReportListener;
+import org.apache.maven.surefire.api.testset.TestSetFailedException;
+import org.apache.maven.surefire.report.RunStatistics;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Computer;
import org.junit.runner.JUnitCore;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.HashMap;
+import java.util.Map;
+
Review comment:
As it seems I used another checkstyle profile in IDEA or I used Maven profile with old content.
I will make a new commit for this.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805575480
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Currently, we have to create a new instance of the listener via `createReporter()` because the Provider `surefire-junit47` has own instance in ThreadLocal. After this has finished in [SUREFIRE-1643](https://issues.apache.org/jira/browse/SUREFIRE-1643) and [SUREFIRE-1860](https://issues.apache.org/jira/browse/SUREFIRE-1860), we can refactor `surefire-junit47` where the ThreadLocal would not be needed and the creator of reporter may turn to getter of singleton.
--
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
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#issuecomment-1038574801
@slawekjaranowski
I have made one more commit because both listeners (ForkingRunListener and TestSetRunListener) are a mirror of the same interface in fork/plugin JVM and so they should implement one and the same interface as mandatory. This way we have avoided ugly casting to type. Any combinations of two interfaces out of three (RunListener, TestOutputReceiver, ConsoleLogger) are avoided and the only TestReportListener is used. Removed ConsoleStream interface and used ConsoleLogger instead. Simplified code around logger in JUnitCoreProvider. Renamed method in ReporterFactory. Added Javadoc explaining the arcitecture, see the interface `TestReportListener`.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805531910
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
The data flow `fork` -> `plugin` is all about reporting events.
The data flow `plugin` -> `fork` is about commands.
So this is Command Event based architecture. Similar wordings are in the enterprise world CQRS and Event Sourcing.
The first data flow usually sends objects in the form of:
- new XxxEvent(new ReportEntry()) - typically test success
- new XxxEvent(<string>) - typically std/out from a Thread of a running test
- new XxxEvent(<string>) - typically console error, debug
Therefore there is such a wording `report` because the provider (InPlugin or fork) sends a report back to the plugin, and the provider is under the plugin in the hierarchy because the plugin is a manager of providers.
Now it is visible in the inheritance
`ForkingRunListener` -> `RunListener`, `TestOutputReceiver`, `ConsoleLogger`.
These are 3 types and three interfaces.
I know that you have a problem with the name of class `ForkingRunListener` but it is not mandatory for the developers because the developers and their code operates with abstractions and their code instantiates the object somewhere but the interfaces are use everywhere. The interfaces have different purpose because we do not want `ParallelComputer` to send TEST_SUCCESS of course not, and the PC sends console internal error if any. This is called information hiding. **So there are reports, all these three interfaces are about reports but different group of reports.**
Regarding `ForkingRunListener`, whatsoever, do you like `ForkingReportListener` more?
From the history, you should know how the code was before. These methods of reports were everywhere. They did not exist one place like it is now in one hotspot. Their implementation was really everywhere, the encoder did not exist in one place and so the encoder's logic was everywhere and it was very hard to maintain the code. Naturally, this situation happened because we are OSS, and in the OSS you start from simple use cases and you do not have time to think of encapsulation, information hiding, and such things cost of maintenance, and this results in paches and not real fixes.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805531910
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
The data flow `fork` -> `plugin` is all about reporting events.
The data flow `plugin` -> `fork` is about commands.
So this is Command Event based architecture. Similar wordings are in the enterprise world CQRS and Event Sourcing.
The first data flow usually sends objects in the form of:
- new XxxEvent(new ReportEntry()) - typically test success
- new XxxEvent(<string>) - typically std/out from a Thread of a running test
- new XxxEvent(<string>) - typically console error, debug
Therefore there is such a wording `report` because the provider (InPlugin or fork) sends a report back to the plugin, and the provider is under the plugin in the hierarchy because the plugin is a manager of providers.
Now it is visible in the inheritance
`ForkingRunListener` -> `RunListener`, `TestOutputReceiver`, `ConsoleLogger`.
These are 3 types and three interfaces.
I know that you have a problem with the name of class `ForkingRunListener` but it is not mandatory for the developers because the developers and their code operates with abstractions and their code instantiates the object somewhere but the interfaces are use everywhere. The interfaces have different purpose because we do not want `ParallelComputer` to send TEST_SUCCESS of course not, and the PC sends console internal error if any. This is called information hiding. **So there are reports, all these three interfaces are about reports but different group of reports.**
Regarding `ForkingRunListener`, whatsoever, do you like `ForkingReportListener` more?
From the history, you should know how the code was before. These methods of reports were everywhere. They did not exist one place like it is now in one hotspot. Their implementation was really everywhere, the encoder did not exist in one place and so the encoder's logic was everywhere and it was very hard to maintain the code. Naturally, this situation happened because we are OSS, and in the OSS you start from simple use cases and you do not have time to think of encapsulation, information hiding, and such things cost of maintenance, and such style results in paches and but nit in real fixes.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806592706
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SurefireReflectorTest.java
##########
@@ -55,7 +55,7 @@ public void testShouldCreateFactoryWithoutException()
ReporterFactory factory = new ReporterFactory()
Review comment:
This PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
--
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
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805212066
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/ReporterFactory.java
##########
@@ -33,7 +33,7 @@
*
* @return A reporter instance
*/
- RunListener createReporter();
+ TestRunListener createReporter();
Review comment:
rename method `createReporter` -> `crateTestRunListener` WDYT?
javadoc for this method is still actual?
##########
File path: surefire-providers/common-junit4/src/main/java/org/apache/maven/surefire/common/junit4/JUnit4RunListener.java
##########
@@ -41,12 +43,14 @@
*
*/
public class JUnit4RunListener
- extends org.junit.runner.notification.RunListener
+ extends RunListener
+ implements TestOutputReceiver
{
- protected final RunListener reporter;
+ protected final TestRunListener reporter;
Review comment:
maybe also change reported -> testRunListener
and in other similar place
##########
File path: surefire-providers/surefire-junit47/src/main/java/org/apache/maven/surefire/junitcore/ConcurrentRunListener.java
##########
@@ -61,10 +60,10 @@
this.reportImmediately = reportImmediately;
this.classMethodCounts = classMethodCounts;
this.consoleStream = consoleStream;
- reporterManagerThreadLocal = new ThreadLocal<RunListener>()
+ reporterManagerThreadLocal = new ThreadLocal<TestRunListener>()
{
@Override
- protected RunListener initialValue()
+ protected TestRunListener initialValue()
{
return reporterFactory.createReporter();
}
Review comment:
now can be like:
```
reporterManagerThreadLocal = ThreadLocal.withInitial( reporterFactory::createReporter );
```
TestSetFailedException is not thrown from this constructor - line 58
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
still we need implements `ConsoleLogger`?
class `TestSetRunListener` is created in one place in `DefaultReporterFactory#createReporter` as `TestRunListener` ...
##########
File path: surefire-api/src/main/java/org/apache/maven/surefire/api/report/TestRunListener.java
##########
@@ -0,0 +1,28 @@
+package org.apache.maven.surefire.api.report;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ *
+ */
+public interface TestRunListener
+ extends RunListener, TestOutputReceiver
+{
+}
Review comment:
New interface - it will be good to add javadoc with description what purpose of this interface is.
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Ok, I can wrong
On fork side
`ForkingRunListener` is used as ConsoleLogger
instance of `ForkingRunListener` is created by `ForkingReporterFactory#createReporter`
in `ForkedBooter` I see
```
forkingReporterFactory = createForkingReporterFactory();
logger = (ConsoleLogger) forkingReporterFactory.createReporter();
```
but when I see `ConsoleLoggerDecorator` - everything can happen in runtime
so don't spend more time on this ...
maybe after more time it will be more clear for me
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805506245
##########
File path: surefire-providers/surefire-junit47/src/main/java/org/apache/maven/surefire/junitcore/ConcurrentRunListener.java
##########
@@ -61,10 +60,10 @@
this.reportImmediately = reportImmediately;
this.classMethodCounts = classMethodCounts;
this.consoleStream = consoleStream;
- reporterManagerThreadLocal = new ThreadLocal<RunListener>()
+ reporterManagerThreadLocal = new ThreadLocal<TestRunListener>()
{
@Override
- protected RunListener initialValue()
+ protected TestRunListener initialValue()
{
return reporterFactory.createReporter();
}
Review comment:
done
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805221122
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
Yes of course becasue the console logger is used to report errors from JVM back to plugin JVM in cases when internal code throws unexpected exception (not the 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805531910
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
The data flow `fork` -> `plugin` is all about reporting events.
The data flow `plugin` -> `fork` is about commands.
So this is Command Event based architecture. Similar wordings are in the enterprise world CQRS and Event Sourcing.
The first data flow usually sends objects in the form of:
- new XxxEvent(new ReportEntry()) - typically test success
- new XxxEvent(<string>) - typically std/out from a Thread of a running test
- new XxxEvent(<string>) - typically console error, debug
Therefore there is such a wording `report` because the provider (InPlugin or fork) sends a report back to the plugin, and the provider is under the plugin in the hierarchy because the plugin is a manager of providers.
Now it is visible in the inheritance
`ForkingRunListener` -> `RunListener`, `TestOutputReceiver`, `ConsoleLogger`.
These are 3 types and three interfaces.
I know that you have a problem with the name of class `ForkingRunListener` but it is not mandatory for the developers because the developers and their code operates with abstractions and their code instantiates the object somewhere but the interfaces are use everywhere. The interfaces have different purpose because we do not want `ParallelComputer` to send TEST_SUCCESS of course not, and the PC sends console internal error if any. This is called information hiding. **So there are reports, all these three interfaces are about reports but different group of reports.**
Regarding `ForkingRunListener`, whatsoever, do you like `ForkingReportListener` more?
From the history, you should know how the code was before. These methods of reports were everywhere. They did not have one place like it is not. Their implementation was really everywhere, the encoder did not exist in one place and so it was very hard to maintain the code. Naturally this situation happened because we are OSS, and in the OSS you start from simple use cases and you do not have time to think of encapsulation, information hiding, and such things.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r805568101
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java
##########
@@ -51,13 +50,13 @@
* @author Kristian Rosenvold
*/
public class TestSetRunListener
- implements RunListener, ConsoleOutputReceiver, ConsoleLogger
+ implements TestRunListener, ConsoleLogger
Review comment:
@slawekjaranowski
This line of code `logger = (ConsoleLogger) forkingReporterFactory.createReporter();` does not exist in the second commit.
--
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #469: [SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #469:
URL: https://github.com/apache/maven-surefire/pull/469#discussion_r806589916
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/output/ForkClient.java
##########
@@ -423,14 +424,14 @@ public final RunListener getReporter()
return getTestSetReporter();
}
- public ConsoleOutputReceiver getConsoleOutputReceiver()
+ public TestOutputReceiver getConsoleOutputReceiver()
{
- return (ConsoleOutputReceiver) getTestSetReporter();
+ return getTestSetReporter();
}
private ConsoleLogger getOrCreateConsoleLogger()
{
- return (ConsoleLogger) getTestSetReporter();
+ return getTestSetReporter();
}
Review comment:
Because this PR has only limited scope of refactoring. This PR is aming for refactoring of the interfaces of the listeners and the reason is the fact that we are not able to associate system output with the Thread of the test without refactoring of these interfaces.
--
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