[GitHub] [maven-surefire] winglam opened a new pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] winglam opened a new pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox

winglam opened a new pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309


   The changes in this pull request addresses issue SUREFIRE-756. Namely, the changes include
   
   - Output of the random seed used to generate a particular random test order when -Dsurefire.runOrder=random is set
   - Ability to replay a previously observed random test order by setting -Dsurefire.seed and -Dfailsafe.seed to the seed that observed the random test order
   - Tests to ensure that the setting of the _same_ random seeds do create the _same_ test orders and _different_ random seeds do create _different_ test orders. Note that the inherent randomness of the orders does mean the tests can be flaky (nondeterministically pass or fail without changes to the code). The current tests have a rate of 0.4% ```(1/3)^5``` of failing. Increasing the number of tests (3) or the number of times to loop (5) would decrease the odds of the test failing.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox

Tibor17 commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464645318



##########
File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java
##########
@@ -310,6 +310,22 @@
     @Parameter( property = "failsafe.runOrder", defaultValue = "filesystem" )
     private String runOrder;
 
+    /**
+     * Sets the random seed that will be used to order the tests if {@code failsafe.runOrder} is set to {@code random}.
+     * <br>
+     * <br>
+     * If no seeds are set and {@code failsafe.runOrder} is set to {@code random}, then the seed used will be
+     * outputted (search for "To reproduce ordering use flag -Dfailsafe.seed").
+     * <br>
+     * <br>
+     * To deterministically reproduce any random test order that was run before, simply set the seed to
+     * be the same value.
+     *
+     * @since 3.0.0-M6
+     */
+    @Parameter( property = "failsafe.seed" )
+    private Long randomSeed;

Review comment:
       Did you take over the changes from an old PR?
   We spoke about this parameter that we already have the config parameter named `runOrder`. And so that it should contain the seed too, let's suppose `<runOrder>random1234567890</runOrder>`.
   The reason is that we have too many config parameters and this one is not urgent due to we are able to specify the seed in one business parameter `runOrder`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox
In reply to this post by GitBox

Tibor17 commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464646617



##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3051,6 +3056,17 @@ private void warnIfIllegalTempDir() throws MojoFailureException
         }
     }
 
+    private void printDefaultSeedIfNecessary()

Review comment:
       This abstract MOJO class is used in the Mockito tests. Can you cover all your changes and extend the existing tests? It would be cool to have tests maybe for larger methods too.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] winglam commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox
In reply to this post by GitBox

winglam commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464647763



##########
File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java
##########
@@ -310,6 +310,22 @@
     @Parameter( property = "failsafe.runOrder", defaultValue = "filesystem" )
     private String runOrder;
 
+    /**
+     * Sets the random seed that will be used to order the tests if {@code failsafe.runOrder} is set to {@code random}.
+     * <br>
+     * <br>
+     * If no seeds are set and {@code failsafe.runOrder} is set to {@code random}, then the seed used will be
+     * outputted (search for "To reproduce ordering use flag -Dfailsafe.seed").
+     * <br>
+     * <br>
+     * To deterministically reproduce any random test order that was run before, simply set the seed to
+     * be the same value.
+     *
+     * @since 3.0.0-M6
+     */
+    @Parameter( property = "failsafe.seed" )
+    private Long randomSeed;

Review comment:
       These changes were not taken from an old PR, but I would be happy to take a look and compare the changes if you can point me to the PR.
   
   As for not adding failsafe.seed or surefire.seed, I can change it so that it just parses the numbers after random as the seed instead. Do let me know if that's what is desired here.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox
In reply to this post by GitBox

Tibor17 commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464650686



##########
File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java
##########
@@ -310,6 +310,22 @@
     @Parameter( property = "failsafe.runOrder", defaultValue = "filesystem" )
     private String runOrder;
 
+    /**
+     * Sets the random seed that will be used to order the tests if {@code failsafe.runOrder} is set to {@code random}.
+     * <br>
+     * <br>
+     * If no seeds are set and {@code failsafe.runOrder} is set to {@code random}, then the seed used will be
+     * outputted (search for "To reproduce ordering use flag -Dfailsafe.seed").
+     * <br>
+     * <br>
+     * To deterministically reproduce any random test order that was run before, simply set the seed to
+     * be the same value.
+     *
+     * @since 3.0.0-M6
+     */
+    @Parameter( property = "failsafe.seed" )
+    private Long randomSeed;

Review comment:
       > Do let me know if that's what is desired here.
   
   Yes this is what I mean. Basically, it is easy to parse the number. You can use regex or the string spliterator.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox
In reply to this post by GitBox

Tibor17 commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464650686



##########
File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java
##########
@@ -310,6 +310,22 @@
     @Parameter( property = "failsafe.runOrder", defaultValue = "filesystem" )
     private String runOrder;
 
+    /**
+     * Sets the random seed that will be used to order the tests if {@code failsafe.runOrder} is set to {@code random}.
+     * <br>
+     * <br>
+     * If no seeds are set and {@code failsafe.runOrder} is set to {@code random}, then the seed used will be
+     * outputted (search for "To reproduce ordering use flag -Dfailsafe.seed").
+     * <br>
+     * <br>
+     * To deterministically reproduce any random test order that was run before, simply set the seed to
+     * be the same value.
+     *
+     * @since 3.0.0-M6
+     */
+    @Parameter( property = "failsafe.seed" )
+    private Long randomSeed;

Review comment:
       > Do let me know if that's what is desired here.
   
   Yes this is what I mean. Basically, it is easy to parse the number. You can use regex or a custom string spliterator.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] winglam commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox
In reply to this post by GitBox

winglam commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464651066



##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3051,6 +3056,17 @@ private void warnIfIllegalTempDir() throws MojoFailureException
         }
     }
 
+    private void printDefaultSeedIfNecessary()

Review comment:
       I can certainly add a test or two that covers this method. If there are other methods that I should cover or if there are particular tests that I should extend, do let me know.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox
In reply to this post by GitBox

Tibor17 commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464655894



##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3051,6 +3056,17 @@ private void warnIfIllegalTempDir() throws MojoFailureException
         }
     }
 
+    private void printDefaultSeedIfNecessary()

Review comment:
       These are the test classes, `AbstractSurefireMojoJava7PlusTest` and `AbstractSurefireMojoTest`, where we add the tests for `AbstractSurefireMojo`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox
In reply to this post by GitBox

Tibor17 commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464657345



##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/booterclient/BooterDeserializerProviderConfigurationTest.java
##########
@@ -276,7 +276,7 @@ private ProviderConfiguration getTestProviderConfiguration( DirectoryScannerPara
             new TestRequest( getSuiteXmlFileStrings(), getTestSourceDirectory(),
                              new TestListResolver( USER_REQUESTED_TEST + "#aUserRequestedTestMethod" ),
                     RERUN_FAILING_TEST_COUNT );
-        RunOrderParameters runOrderParameters = new RunOrderParameters( RunOrder.DEFAULT, null );
+        RunOrderParameters runOrderParameters = new RunOrderParameters( RunOrder.DEFAULT, null, null );

Review comment:
       It would be worth to test the non-null value.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox
In reply to this post by GitBox

Tibor17 commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464657488



##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/booterclient/BooterDeserializerStartupConfigurationTest.java
##########
@@ -197,7 +197,7 @@ private ProviderConfiguration getProviderConfiguration()
             new TestRequest( Arrays.asList( getSuiteXmlFileStrings() ), getTestSourceDirectory(),
                              new TestListResolver( "aUserRequestedTest#aUserRequestedTestMethod" ) );
 
-        RunOrderParameters runOrderParameters = new RunOrderParameters( RunOrder.DEFAULT, null );
+        RunOrderParameters runOrderParameters = new RunOrderParameters( RunOrder.DEFAULT, null, null );

Review comment:
       detto here




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #309: [SUREFIRE-756] Allow ability to capture executed random runOrder for re-play purposes

GitBox
In reply to this post by GitBox

Tibor17 commented on a change in pull request #309:
URL: https://github.com/apache/maven-surefire/pull/309#discussion_r464659711



##########
File path: surefire-booter/src/main/java/org/apache/maven/surefire/booter/SurefireReflector.java
##########
@@ -178,10 +178,11 @@ private Object createRunOrderParameters( RunOrderParameters runOrderParameters )
             return null;
         }
         //Can't use the constructor with the RunOrder parameter. Using it causes some integration tests to fail.
-        Class<?>[] arguments = { String.class, File.class };
+        Class<?>[] arguments = { String.class, File.class, Long.class };
         Constructor<?> constructor = getConstructor( this.runOrderParameters, arguments );
         File runStatisticsFile = runOrderParameters.getRunStatisticsFile();
-        return newInstance( constructor, RunOrder.asString( runOrderParameters.getRunOrder() ), runStatisticsFile );
+        return newInstance( constructor, RunOrder.asString( runOrderParameters.getRunOrder() ), runStatisticsFile,

Review comment:
       This reflector should be tested in `SurefireReflectorTest`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]