[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r846545476
##
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##
@@ -3114,9 +3114,8 @@ protected void warnIfIllegalFailOnFlakeCount() throws
MojoFailureException
private void printDefaultSeedIfNecessary()
{
-if ( getRunOrderRandomSeed() == null && getRunOrder().equals(
RunOrder.RANDOM.name() ) )
+if ( getRunOrder().equals( RunOrder.RANDOM.name() ) )
{
-setRunOrderRandomSeed( System.nanoTime() );
Review Comment:
I think you wanted to have the following:
```
if ( getRunOrderRandomSeed() == null )
{
setRunOrderRandomSeed( System.nanoTime() );
}
```
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844543780
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
@delanym I made `RunOrderParameters` immutable because the contributor did
not implement it right and the setter was not necessary. Pls rebase your branch
on `origin/master`. You should exclude the fallback value `System.nanoTime()`
in the `DefaultRunOrderCalculator` constructor and use it only in MOJO. It does
not make sense to do one thing twice with the seed.
Finally `this.random = new Random( runOrderRandomSeed == null ?
System.nanoTime() : runOrderRandomSeed );` would become `random =
runOrderRandomSeed == null ? null : new Random( runOrderRandomSeed );`.
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844516213
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
@slawekjaranowski Do you remember all the years we solved together with
Robert and Infra team because the Windows paths were limited on the file
system? They are still limitted however the limit is longer but it still
exists! Also the infra team prolonged the limit of CMD line in bash but it
still exists.
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844548223
##
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##
@@ -3114,9 +3114,12 @@ protected void warnIfIllegalFailOnFlakeCount() throws
MojoFailureException
private void printDefaultSeedIfNecessary()
{
-if ( getRunOrderRandomSeed() == null && getRunOrder().equals(
RunOrder.RANDOM.name() ) )
+if ( getRunOrder().equals( RunOrder.RANDOM.name() ) )
{
-setRunOrderRandomSeed( System.nanoTime() );
+if ( getRunOrderRandomSeed() == null )
+{
+setRunOrderRandomSeed( System.nanoTime() );
Review Comment:
@slawekjaranowski runOrder can be other than random and so
`DefaultRunOrderCalculator` may obtain null seed.
@delanym see my comment
https://github.com/apache/maven-surefire/pull/505#discussion_r844543780
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844543780
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
@delanym I made `RunOrderParameters` immutable because the contributor did
not implement it right and the setter was not necessary. Pls rebase you branch
on `origin/master`. You should exclude the fallback value `System.nanoTime()`
in the `DefaultRunOrderCalculator` constructor and use it only in MOJO. It does
not make sense to do one thing twice with the seed.
Finally `this.random = new Random( runOrderRandomSeed == null ?
System.nanoTime() : runOrderRandomSeed );` would become `random =
runOrderRandomSeed == null ? null : new Random( runOrderRandomSeed );`.
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844543780
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
@delanym I made `RunOrderParameters` immutable because the contributor did
not implement it right and the setter was not necessary. Pls rebase you branch
on `origin/master`. You should exclude the fallback value `System.nanoTime()`
in the `DefaultRunOrderCalculator` constructor and use it only in MOJO. It does
not make sense to do one thing twice with the seed.
Finally `this.random = new Random( runOrderRandomSeed == null ?
System.nanoTime() : runOrderRandomSeed );` would become `this.random =
runOrderRandomSeed == null ? null : new Random( runOrderRandomSeed );`.
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844543780
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
@delanym I made `RunOrderParameters` immutable because the contributor did
not implement it right and the setter was not necessary. You should exclude the
fallback value `System.nanoTime()` in the `DefaultRunOrderCalculator`
constructor and use it only in MOJO. It does not make sense to do one thing
twice with the seed.
Finally `this.random = new Random( runOrderRandomSeed == null ?
System.nanoTime() : runOrderRandomSeed );` would become `this.random =
runOrderRandomSeed == null ? null : new Random( runOrderRandomSeed );`.
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844543780
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
@delanym I made `RunOrderParameters` immutable because the contributor did
not implement it right and the setter was not necessary. You should exclude the
fallback value `System.nanoTime()` in the `DefaultRunOrderCalculator`
constructor and use it only in MOJO. It does not make sense to do one thing
twice with the seed.
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844516213
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
@slawekjaranowski Do you remember all the years we solved together with
Robert and Infra team because the Windows paths were limited on the file
system? They are!
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844514843
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
@delanym
We are doing these things twice. The seed is set to `System.nanoTime()` if
it was NULL, see `DefaultRunOrderCalculator`.
This means that the only work here should be different, we should change the
condition in the IF statement but nothing more. WDYT? Pls comment on this!
```
Long runOrderRandomSeed = runOrderParameters.getRunOrderRandomSeed();
if ( runOrderRandomSeed == null )
{
runOrderRandomSeed = System.nanoTime();
runOrderParameters.setRunOrderRandomSeed( runOrderRandomSeed );
}
this.random = new Random( runOrderRandomSeed );
```
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844514843
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
@delanym
We are doing these things twice. The seed is set to `System.nanoTime()` if
it was NULL, see `DefaultRunOrderCalculator`.
This means that the only work here is to change the condition in the IF
statement but nothing more!
```
Long runOrderRandomSeed = runOrderParameters.getRunOrderRandomSeed();
if ( runOrderRandomSeed == null )
{
runOrderRandomSeed = System.nanoTime();
runOrderParameters.setRunOrderRandomSeed( runOrderRandomSeed );
}
this.random = new Random( runOrderRandomSeed );
```
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844229724
##
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##
@@ -3114,9 +3114,12 @@ protected void warnIfIllegalFailOnFlakeCount() throws
MojoFailureException
private void printDefaultSeedIfNecessary()
{
-if ( getRunOrderRandomSeed() == null && getRunOrder().equals(
RunOrder.RANDOM.name() ) )
+if ( getRunOrder().equals( RunOrder.RANDOM.name() ) )
{
-setRunOrderRandomSeed( System.nanoTime() );
+if ( getRunOrderRandomSeed() == null )
+{
+setRunOrderRandomSeed( System.nanoTime() );
Review Comment:
Let's see the implementation, because the impl has much better opportunities
to use real random number if this parameter is NULL.
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844226860
##
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##
@@ -3114,9 +3114,12 @@ protected void warnIfIllegalFailOnFlakeCount() throws
MojoFailureException
private void printDefaultSeedIfNecessary()
{
-if ( getRunOrderRandomSeed() == null && getRunOrder().equals(
RunOrder.RANDOM.name() ) )
+if ( getRunOrder().equals( RunOrder.RANDOM.name() ) )
{
-setRunOrderRandomSeed( System.nanoTime() );
+if ( getRunOrderRandomSeed() == null )
+{
+setRunOrderRandomSeed( System.nanoTime() );
Review Comment:
Personally I would expect some warning that the seed is missing.
If random, then yes, we should use random and prevent from any cohesion
between the neighbouring runs.
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #505: [SUREFIRE-2055] Always show random seed
Tibor17 commented on code in PR #505:
URL: https://github.com/apache/maven-surefire/pull/505#discussion_r844223314
##
surefire-its/src/test/java/org/apache/maven/surefire/its/RunOrderIT.java:
##
@@ -97,7 +97,17 @@ public void testRandomJUnit4SameSeed()
}
}
}
-
+
+@Test
+public void testRandomJUnit4PrintSeed()
+{
+long seed = 0L;
+OutputValidator validator = executeWithRandomOrder( "junit4", seed );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
+validator = executeWithRandomOrder( "junit4" );
+validator.verifyTextInLog( "To reproduce ordering use flag" );
Review Comment:
Hi @delanym . Nice to meet you after the release Vote!
I know that it is a matter of taste but these names of classes and methods
determine the length of folders on the file system in our CI. Using `WithSeed`
and `WithoutSeed` is shorter.
Now, I have realized that @delanym is using two executions which might be
utilized in some way.
@delanym what wat your point? You wanted to compare the XML reports and the
order of tests? Something else? If you want to make sure that the seed is set,
such situation can be verified with the old IT where a debug message can be
printed in the logs and the IT can check that the message was printed with seed
!= NULL.
--
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-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
