[SUREFIRE-2011] Updated abstractions which helps associating standard out/err with a test #469
Conversation
| /** | ||
| * | ||
| */ | ||
| public interface TestRunListener | ||
| extends RunListener, TestOutputReceiver | ||
| { | ||
| } |
There was a problem hiding this comment.
New interface - it will be good to add javadoc with description what purpose of this interface is.
| reporterManagerThreadLocal = new ThreadLocal<RunListener>() | ||
| reporterManagerThreadLocal = new ThreadLocal<TestRunListener>() | ||
| { | ||
| @Override | ||
| protected RunListener initialValue() | ||
| protected TestRunListener initialValue() | ||
| { | ||
| return reporterFactory.createReporter(); | ||
| } |
There was a problem hiding this comment.
now can be like:
reporterManagerThreadLocal = ThreadLocal.withInitial( reporterFactory::createReporter );
TestSetFailedException is not thrown from this constructor - line 58
| { | ||
| protected final RunListener reporter; | ||
| protected final TestRunListener reporter; |
There was a problem hiding this comment.
maybe also change reported -> testRunListener
and in other similar place
There was a problem hiding this comment.
The interface TestReportListener contains the wording report, pls see the next commit.
| @@ -51,13 +50,13 @@ | |||
| * @author Kristian Rosenvold | |||
| */ | |||
| public class TestSetRunListener | |||
| implements RunListener, ConsoleOutputReceiver, ConsoleLogger | |||
| implements TestRunListener, ConsoleLogger | |||
There was a problem hiding this comment.
still we need implements ConsoleLogger?
class TestSetRunListener is created in one place in DefaultReporterFactory#createReporter as TestRunListener ...
There was a problem hiding this 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).
There was a problem hiding this 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
There was a problem hiding this 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() - typically std/out from a Thread of a running test
- new XxxEvent() - 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.
There was a problem hiding this 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.
There was a problem hiding this 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 and 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.
There was a problem hiding this 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.
| @@ -33,7 +33,7 @@ | |||
| * | |||
| * @return A reporter instance | |||
| */ | |||
| RunListener createReporter(); | |||
| TestRunListener createReporter(); | |||
There was a problem hiding this comment.
rename method createReporter -> crateTestRunListener WDYT?
javadoc for this method is still actual?
There was a problem hiding this comment.
@slawekjaranowski yes, the method was renamed in the second commit.
Tibor17
force-pushed
the
test-run-id2
branch
from
0f80934
to
320c0ca
Compare
|
@slawekjaranowski |
Tibor17
force-pushed
the
test-run-id2
branch
2 times, most recently
from
d592f99
to
23ed82a
Compare
| private ConsoleLogger getOrCreateConsoleLogger() | ||
| { | ||
| return (ConsoleLogger) getTestSetReporter(); | ||
| return getTestSetReporter(); | ||
| } |
There was a problem hiding this comment.
why not use getTestSetReporter() instead of getOrCreateConsoleLogger() - private method
There was a problem hiding this 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.
| @@ -55,7 +55,7 @@ public void testShouldCreateFactoryWithoutException() | |||
| ReporterFactory factory = new ReporterFactory() | |||
There was a problem hiding this comment.
can be mock, only reference of object is checked.
There was a problem hiding this 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.
| @@ -255,7 +255,7 @@ public void testReporterFactory() | |||
| ReporterFactory reporterFactory = new ReporterFactory() | |||
There was a problem hiding this comment.
can be mock
--
Surprise - what is foo? 😄 line 245
There was a problem hiding this 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.
| @@ -279,7 +279,7 @@ public void testReporterFactoryAware() | |||
| ReporterFactory reporterFactory = new ReporterFactory() | |||
There was a problem hiding this 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.reporter = reporter; | ||
| } | ||
|
|
||
| public final ConsoleLogger getConsoleLogger() |
There was a problem hiding this comment.
new public method without Overrides .... some of comments will be useful
There was a problem hiding this 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.
| <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> |
There was a problem hiding this comment.
super ... I even didn't try understand it
There was a problem hiding this 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.
| 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; | ||
|
|
There was a problem hiding this comment.
There was a problem hiding this 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.
| 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; |
… out/err with a test
Tibor17
force-pushed
the
test-run-id2
branch
from
2ee084f
to
3bb83c4
Compare
The method
void writeTestOutput( String output, boolean newLine, boolean stdout )appeared inForkingRunListenerandTestSetRunListenerand it was called byConsoleOutputCapture.The information (
testRunIdand Thread) is associated with the particular run of the test method in the implementation of provider'sRunListener. So the listener has this information, and not theForkingRunListener.Due to we will implement enum
RunModeandtestRunId:longwe should not redirectvoid writeTestOutput( String output, boolean newLine, boolean stdout )toForkingRunListener. Therefore we created the interfaceTestRunListenerwhich 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. 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:
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.
[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,where you replace
SUREFIRE-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean installto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
mvn -Prun-its clean install).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.