New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SUREFIRE-1926] Console logs should be synchronized #419
Conversation
e45f6d1
to
34f5ffb
Compare
@slawekjaranowski |
@@ -206,7 +207,7 @@ public void run() | |||
// if tests failed, but if this does not happen then printing warning to console is the only way to | |||
// inform the users. | |||
String msg = "ForkStarter IOException: " + e.getLocalizedMessage() + "."; | |||
File reportsDir = defaultReporterFactory.getReportsDirectory(); | |||
File reportsDir = reportMerger.getReportsDirectory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not
startupReportConfiguration.getReportsDirectory();
than we can remove this method from ReportsMerger
interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there must be abstraction. There are two DefaultReporterFactory instances. One is per fork, and another is the "merger".
We simply cannot allow ForkStarter or the developer who is not aware of Surefire insides to call other methods than the "merger" can expose.
Now, the answer for the following questions why this interface ReportsMerger exists in here is the fact that I implement the method writeTestOutput(). Of course I cannot allow the "merger" to call this method by any reason even if by mistake. So the ReportsMerger becomes a subinterface with limitted access excluding writeTestOutput() to prevent from calling it, to prevent from calling new implementation of this method, so that it will be called only in ForkClient and not in ForkStarter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reportMerger
and startupReportConfiguration
are final variable
startupReportConfiguration
is pass to reportMerger
constructor ...
CloseableCloser
is inner class which use variable from ForkStarter instance
is it possible that we have different instance of startupReportConfiguration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I don't see something ...
@@ -65,7 +65,7 @@ | |||
* @author Kristian Rosenvold | |||
*/ | |||
public class DefaultReporterFactory | |||
implements ReporterFactory | |||
implements ReporterFactory, ReportsMerger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next responsibility for this class - maybe will be possible create separate class
void runStarting(); | ||
void mergeFromOtherFactories( Collection<DefaultReporterFactory> factories ); | ||
File getReportsDirectory(); | ||
RunResult close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name of method is not corresponding to what is doing ... inside close
method - there are calling
mergeTestHistoryResult
runCompleted
- and
close
on all listerneres colected inmergeFromOtherFactories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The close method is closing the status, no matter that the class name is factory, it really does closing the reports after JVM exit. But I do not want to spend time on talking about theory with design patterns. I was aiming for not giving the developer a chance to call the method I am reimplementing since this class is polymorphic - it has two usages in ForkStarter.
I'm still not comfortable for adding next responsibility |
@slawekjaranowski If you see the usages of Of course it is possible to make a refactoring and rename few methods, which is your main whish coming from your feeling, but it is not the goal of this ticket. The fact is that the field |
The point is to get logger operations in semi-sequential order across multiple threads.
The messages will not be mixed up, especially out and err.
PluginConsoleLogger
is synchronized. This instance is only one and it is passed toDefaultReporterFactory
andTestSetRunListener
as a monitor object.