[GitHub] maven-surefire pull request #127: [SUREFIRE-1293] Simplify org.apache.maven....
Github user britter closed the pull request at: https://github.com/apache/maven-surefire/pull/127 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
[GitHub] maven-surefire pull request #127: [SUREFIRE-1293] Simplify org.apache.maven....
Github user Tibor17 commented on a diff in the pull request: https://github.com/apache/maven-surefire/pull/127#discussion_r82700832 --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java --- @@ -76,7 +76,7 @@ public TestSetRunListener( ConsoleReporter consoleReporter, FileReporter fileRep StatisticsReporter statisticsReporter, boolean trimStackTrace, boolean isPlainFormat, boolean briefOrPlainFormat ) { -this.consoleReporter = consoleReporter; +this.consoleReporter = consoleReporter != null ? consoleReporter : new NullConsoleReporter(); --- End diff -- @britter >>does this belong into DefaultReporterFactory.createReporter() If you mean this code: `ConsoleReporter consoleReporter = shouldReportToConsole() ? new ConsoleReporter( consoleLogger ) : null;` Then I would keep the responsibility of creating anew reporter yet in the method `createReporter()`. I am going to fix a bug exactly in this part of code after you. We can still improve the code by small incremental changes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
[GitHub] maven-surefire pull request #127: [SUREFIRE-1293] Simplify org.apache.maven....
Github user Tibor17 commented on a diff in the pull request: https://github.com/apache/maven-surefire/pull/127#discussion_r82699845 --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java --- @@ -76,7 +76,7 @@ public TestSetRunListener( ConsoleReporter consoleReporter, FileReporter fileRep StatisticsReporter statisticsReporter, boolean trimStackTrace, boolean isPlainFormat, boolean briefOrPlainFormat ) { -this.consoleReporter = consoleReporter; +this.consoleReporter = consoleReporter != null ? consoleReporter : new NullConsoleReporter(); --- End diff -- no no, I meant only the scope of this PR. Not whole Surefire of course. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
[GitHub] maven-surefire pull request #127: [SUREFIRE-1293] Simplify org.apache.maven....
Github user Tibor17 commented on a diff in the pull request: https://github.com/apache/maven-surefire/pull/127#discussion_r82699554 --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/NullConsoleReporter.java --- @@ -33,7 +33,10 @@ class NullConsoleReporter extends ConsoleReporter { -NullConsoleReporter() { + +static final NullConsoleReporter INSTANCE = new NullConsoleReporter(); + +private NullConsoleReporter() { --- End diff -- here as well pls run the build. it explores checkstyle erros. Did you get your IntelliJ Idea now working with our Maven checkstyle? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
[GitHub] maven-surefire pull request #127: [SUREFIRE-1293] Simplify org.apache.maven....
Github user Tibor17 commented on a diff in the pull request: https://github.com/apache/maven-surefire/pull/127#discussion_r82699403 --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/NullConsoleOutputReceiver.java --- @@ -31,7 +31,13 @@ implements TestcycleConsoleOutputReceiver { -public void testSetStarting( ReportEntry reportEntry ) +static final NullConsoleOutputReceiver INSTANCE = new NullConsoleOutputReceiver(); + +private NullConsoleOutputReceiver() +{ +} + +public void testSetStarting(ReportEntry reportEntry ) --- End diff -- here is checkstyle error --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
[GitHub] maven-surefire pull request #127: [SUREFIRE-1293] Simplify org.apache.maven....
Github user britter commented on a diff in the pull request: https://github.com/apache/maven-surefire/pull/127#discussion_r82631034 --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/NullStatisticsReporter.java --- @@ -35,7 +36,7 @@ public NullStatisticsReporter() { -super( FileUtils.getTempDirectory() ); +super( FileUtils.getFile( FileUtils.getTempDirectory(), RandomStringUtils.randomAlphabetic( 24 ) ) ); --- End diff -- @Tibor17 makes sense. If we change it like this, we don't need the test to make sure the null objects can be created. I'll modify the PR to incorporate protected default constructors and remove the empty tests. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
[GitHub] maven-surefire pull request #127: [SUREFIRE-1293] Simplify org.apache.maven....
Github user britter commented on a diff in the pull request: https://github.com/apache/maven-surefire/pull/127#discussion_r82631691 --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java --- @@ -76,7 +76,7 @@ public TestSetRunListener( ConsoleReporter consoleReporter, FileReporter fileRep StatisticsReporter statisticsReporter, boolean trimStackTrace, boolean isPlainFormat, boolean briefOrPlainFormat ) { -this.consoleReporter = consoleReporter; +this.consoleReporter = consoleReporter != null ? consoleReporter : new NullConsoleReporter(); --- End diff -- > All of these classes are in surefire-common project. Therefore it would be nice not to create a new object new NullConsoleReporter() but instead keep it as static final constant in abstract Factory in surefire-common. Agreed, I can change this. > I would say that the caller should pass non-null consistent object into this class TestSetRunListener via constructor Yes it would certainly be better to enforce this kinds of invariants. We can do it like this, but then the constructor of `TestSetRunListener` has to check for null parameters and throw an exception if null is passed. > If we accepted such paradigm then all such occurrences in this PR would have to change it. I don't understand. Change the whole surefire code base for all occurrences of this pattern? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
[GitHub] maven-surefire pull request #127: [SUREFIRE-1293] Simplify org.apache.maven....
Github user Tibor17 commented on a diff in the pull request: https://github.com/apache/maven-surefire/pull/127#discussion_r82527079 --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java --- @@ -76,7 +76,7 @@ public TestSetRunListener( ConsoleReporter consoleReporter, FileReporter fileRep StatisticsReporter statisticsReporter, boolean trimStackTrace, boolean isPlainFormat, boolean briefOrPlainFormat ) { -this.consoleReporter = consoleReporter; +this.consoleReporter = consoleReporter != null ? consoleReporter : new NullConsoleReporter(); --- End diff -- All of these classes are in `surefire-common` project. Therefor it would be nice not to create a new object `new NullConsoleReporter()` but instead keep it as static final constant in abstract Factory in surefire-common. This would mean that such null-check may be embedded in the factory, e.g. `buildConsoleReporter()`. I would say that the caller should pass non-null consistent object into this class `TestSetRunListener` via constructor and the called class should not make any guess about it (means if == null => guess hide the error). If we accepted such paradigm then all such occurrences in this PR would have to change it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
[GitHub] maven-surefire pull request #127: [SUREFIRE 1293] Simplify org.apache.maven....
GitHub user britter opened a pull request: https://github.com/apache/maven-surefire/pull/127 [SUREFIRE 1293] Simplify org.apache.maven.plugin.surefire.report.TestSetRunListener by using the null object pattern Added null object implementations for fields used by TestSetRunListener in order to simplify TestSetRunListener code. You can merge this pull request into a Git repository by running: $ git pull https://github.com/britter/maven-surefire SUREFIRE-1293 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/maven-surefire/pull/127.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #127 commit fa5f480fbe02dbd401b98ca39b56a9a0db930f48 Author: Benedikt Ritter Date: 2016-10-09T14:41:49Z Introduce NullConsoleReporter to simplify TesSetRunListener commit a811fb618b4382feff6ee991d253c816f62e7d94 Author: Benedikt Ritter Date: 2016-10-09T14:47:31Z Introduce NullFileReporter to simplify TesSetRunListener commit d419b7be567546fe615bdcd770fe03229c141ca2 Author: Benedikt Ritter Date: 2016-10-09T14:53:34Z Introduce NullStatisticsReporter to simplify TesSetRunListener commit e56269cfce8b626e0e2667982891068251beedf6 Author: Benedikt Ritter Date: 2016-10-09T14:57:16Z Introduce NullStatelessXmlReporter to simplify TesSetRunListener commit 42dd834e098c3bbf8f103db52fb3edc125cd7276 Author: Benedikt Ritter Date: 2016-10-09T15:00:39Z Introduce NullConsoleOutputReceiver to simplify TesSetRunListener commit cbf8e0948a76f82e3ef7c124e21b2edb9b96f636 Author: Benedikt Ritter Date: 2016-10-09T15:07:38Z Make sure all the Null object classes can be instantiated --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org