Remove the optimization which data provider is remembered in a multiplication (#2119)

This could cause unintuitive behavior and changes in test sets that are executed.

Author: Vampire

spock-core/src/main/java/org/spockframework/runtime/DataIteratorFactory.java

@@ -702,16 +702,6 @@ private static class DataProviderMultiplier extends BaseDataIterator {
      */
     private final Object[] multiplierProviders;
 
-    /**
-     * The data provider multiplier that is used as multiplicand
-     * in this multiplication, if it represents in an
-     * {@code (a * (b * c))} multiplication the {@code (a * bc)} part
-     * or otherwise {@code null}.
-     * Outside the constructor it is only used as condition
-     * and for the final cleanup,
-     */
-    private final DataProviderMultiplier multiplicandProvider;
-
     /**
      * The data providers that are used as multiplicand
      * in this multiplication, if it represents in an
@@ -739,12 +729,6 @@ private static class DataProviderMultiplier extends BaseDataIterator {
      */
     private final Iterator<?>[] multiplicandIterators;
 
-    /**
-     * A flag that remembers whether the factors were swapped for optimization, so that the result values can be
-     * swapped back for expected, consistent, and reproducible results.
-     */
-    private boolean factorsSwapped = false;
-
     /**
      * The current set of multiplier values that is combined with each set of multiplicand values,
      * before the next multiplier value is calculated.
@@ -794,8 +778,15 @@ public DataProviderMultiplier(IRunSupervisor supervisor, SpockExecutionContext c
                                   Object[] multiplierProviders, Object[] multiplicandProviders) {
       super(supervisor, context);
       this.dataVariableNames = Objects.requireNonNull(dataVariableNames);
+      this.multiplierProviderInfos = multiplierProviderInfos;
+      this.multiplicandProviderInfos = multiplicandProviderInfos;
       multiplierProvider = null;
-      multiplicandProvider = null;
+      this.multiplierProviders = multiplierProviders;
+      this.multiplicandProviders = multiplicandProviders;
+
+      collectedMultiplicandValues = createMultiplicandStorage();
+      multiplierIterators = createIterators(this.multiplierProviders, this.multiplierProviderInfos);
+      multiplicandIterators = createIterators(this.multiplicandProviders, this.multiplicandProviderInfos);
 
       int estimatedMultiplierIterations = estimateNumIterations(Objects.requireNonNull(multiplierProviders));
       int estimatedMultiplicandIterations = estimateNumIterations(Objects.requireNonNull(multiplicandProviders));
@@ -805,26 +796,6 @@ public DataProviderMultiplier(IRunSupervisor supervisor, SpockExecutionContext c
           : estimatedMultiplierIterations * estimatedMultiplicandIterations;
 
       processedDataProviders = multiplierProviders.length + multiplicandProviders.length;
-
-      if ((estimatedMultiplierIterations != UNKNOWN_ITERATIONS)
-        && ((estimatedMultiplicandIterations == UNKNOWN_ITERATIONS) || (estimatedMultiplicandIterations > estimatedMultiplierIterations))) {
-        // multiplier is not unknown and multiplicand is unknown or larger,
-        // swap factors so that potentially fewer values need to be remembered in memory
-        factorsSwapped = true;
-        this.multiplierProviderInfos = multiplicandProviderInfos;
-        this.multiplicandProviderInfos = multiplierProviderInfos;
-        this.multiplierProviders = multiplicandProviders;
-        this.multiplicandProviders = multiplierProviders;
-      } else {
-        this.multiplierProviderInfos = multiplierProviderInfos;
-        this.multiplicandProviderInfos = multiplicandProviderInfos;
-        this.multiplierProviders = multiplierProviders;
-        this.multiplicandProviders = multiplicandProviders;
-      }
-
-      collectedMultiplicandValues = createMultiplicandStorage();
-      multiplierIterators = createIterators(this.multiplierProviders, this.multiplierProviderInfos);
-      multiplicandIterators = createIterators(this.multiplicandProviders, this.multiplicandProviderInfos);
     }
 
     /**
@@ -845,6 +816,15 @@ public DataProviderMultiplier(IRunSupervisor supervisor, SpockExecutionContext c
                                   DataProviderMultiplier multiplierProvider, Object[] multiplicandProviders) {
       super(supervisor, context);
       this.dataVariableNames = Objects.requireNonNull(dataVariableNames);
+      multiplierProviderInfos = null;
+      this.multiplicandProviderInfos = multiplicandProviderInfos;
+      this.multiplierProvider = multiplierProvider;
+      multiplierProviders = null;
+      this.multiplicandProviders = multiplicandProviders;
+
+      collectedMultiplicandValues = createMultiplicandStorage();
+      multiplierIterators = new Iterator[]{multiplierProvider};
+      multiplicandIterators = createIterators(this.multiplicandProviders, this.multiplicandProviderInfos);
 
       int estimatedMultiplierIterations = Objects.requireNonNull(multiplierProvider).getEstimatedNumIterations();
       int estimatedMultiplicandIterations = estimateNumIterations(Objects.requireNonNull(multiplicandProviders));
@@ -854,32 +834,6 @@ public DataProviderMultiplier(IRunSupervisor supervisor, SpockExecutionContext c
           : estimatedMultiplierIterations * estimatedMultiplicandIterations;
 
       processedDataProviders = multiplierProvider.getProcessedDataProviders() + multiplicandProviders.length;
-
-      if ((estimatedMultiplierIterations != UNKNOWN_ITERATIONS)
-        && ((estimatedMultiplicandIterations == UNKNOWN_ITERATIONS) || (estimatedMultiplicandIterations > estimatedMultiplierIterations))) {
-        // multiplier is not unknown and multiplicand is unknown or larger,
-        // swap factors so that potentially fewer values need to be remembered in memory
-        factorsSwapped = true;
-        multiplierProviderInfos = multiplicandProviderInfos;
-        this.multiplicandProviderInfos = null;
-        this.multiplierProvider = null;
-        multiplierProviders = multiplicandProviders;
-        multiplicandProvider = multiplierProvider;
-        this.multiplicandProviders = null;
-        collectedMultiplicandValues = singletonList(new ArrayList<>());
-        multiplierIterators = createIterators(multiplierProviders, multiplierProviderInfos);
-        multiplicandIterators = new Iterator[]{multiplierProvider};
-      } else {
-        multiplierProviderInfos = null;
-        this.multiplicandProviderInfos = multiplicandProviderInfos;
-        this.multiplierProvider = multiplierProvider;
-        multiplierProviders = null;
-        multiplicandProvider = null;
-        this.multiplicandProviders = multiplicandProviders;
-        collectedMultiplicandValues = createMultiplicandStorage();
-        multiplierIterators = new Iterator[]{multiplierProvider};
-        multiplicandIterators = createIterators(this.multiplicandProviders, this.multiplicandProviderInfos);
-      }
     }
 
     private List<List<Object>> createMultiplicandStorage() {
@@ -892,7 +846,7 @@ private List<List<Object>> createMultiplicandStorage() {
 
     @Override
     public void close() throws Exception {
-      closeQuietly(multiplierProvider, multiplicandProvider);
+      closeQuietly(multiplierProvider);
       closeQuietly(multiplierProviders);
       closeQuietly(multiplicandProviders);
     }
@@ -958,21 +912,11 @@ public Object[] next() {
           collectedMultiplicandValues.get(i).add(nextMultiplicandValues[i]);
         }
       }
-      if (multiplicandProvider != null) {
-        nextMultiplicandValues = ((Object[]) nextMultiplicandValues[0]);
-      }
 
       // prepare result
-      Object[] nextMultiplierValues;
-      if (factorsSwapped) {
-        nextMultiplierValues = nextMultiplicandValues;
-        nextMultiplicandValues = currentMultiplierValues;
-      } else {
-        nextMultiplierValues = currentMultiplierValues;
-      }
-      Object[] next = new Object[nextMultiplierValues.length + nextMultiplicandValues.length];
-      System.arraycopy(nextMultiplierValues, 0, next, 0, nextMultiplierValues.length);
-      System.arraycopy(nextMultiplicandValues, 0, next, nextMultiplierValues.length, nextMultiplicandValues.length);
+      Object[] next = new Object[currentMultiplierValues.length + nextMultiplicandValues.length];
+      System.arraycopy(currentMultiplierValues, 0, next, 0, currentMultiplierValues.length);
+      System.arraycopy(nextMultiplicandValues, 0, next, currentMultiplierValues.length, nextMultiplicandValues.length);
       return next;
     }
 

spock-specs/src/test/groovy/org/spockframework/docs/datadriven/DataSpec.groovy

@@ -200,8 +200,10 @@ class DataSpec extends EmbeddedSpecification {
       tag::single-data-providers-combined-result1[]
       - `feature [a: 1, b: 5, c: 7, #0]`
       - `feature [a: 2, b: 5, c: 8, #1]`
-      - `feature [a: 3, b: 6, c: 7, #2]`
-      - `feature [a: 4, b: 6, c: 8, #3]`
+      - `feature [a: 3, b: 5, c: 9, #2]`
+      - `feature [a: 4, b: 6, c: 7, #3]`
+      - `feature [a: 5, b: 6, c: 8, #4]`
+      - `feature [a: 6, b: 6, c: 9, #5]`
       end::single-data-providers-combined-result1[]
     '''
       .stripIndent(*(GroovyRuntimeUtil.groovy3orNewer ? [true] : []))
@@ -218,10 +220,10 @@ class DataSpec extends EmbeddedSpecification {
 
         // tag::single-data-providers-combined1[]
         where:
-        a << [1, 2, 3, 4]
+        a << [1, 2, 3, 4, 5, 6]
         b << [5, 6]
         combined:
-        c << [7, 8]
+        c << [7, 8, 9]
         // end::single-data-providers-combined1[]
       }
     '''

spock-specs/src/test/groovy/org/spockframework/smoke/parameterization/DataTables.groovy

@@ -316,10 +316,10 @@ class DataTables extends EmbeddedSpecification {
     results.testEvents().started().list().testDescriptor.displayName == [
       'a feature (#a #b #c)',
       'a feature (1 3 4)',
-      'a feature (2 3 4)',
       'a feature (1 3 5)',
-      'a feature (2 3 5)',
       'a feature (1 3 6)',
+      'a feature (2 3 4)',
+      'a feature (2 3 5)',
       'a feature (2 3 6)'
     ]
 

spock-specs/src/test/resources/snapshots/org/spockframework/smoke/parameterization/DataTables/filtered_iterations_are_logged.txt

@@ -1,4 +1,4 @@
-Filtered iteration [i: 2, a: 2, b: 2, c: 1]:
+Filtered iteration [i: 2, a: 1, b: 1, c: 2]:
 Condition not satisfied:
 
 i == 1
@@ -7,7 +7,7 @@ i == 1
 
 	at apackage.ASpec.a feature(script.groovy:15)
 
-Filtered iteration [i: 3, a: 1, b: 1, c: 2]:
+Filtered iteration [i: 3, a: 1, b: 1, c: 3]:
 Condition not satisfied:
 
 i == 1
@@ -16,7 +16,7 @@ i == 1
 
 	at apackage.ASpec.a feature(script.groovy:15)
 
-Filtered iteration [i: 4, a: 2, b: 2, c: 2]:
+Filtered iteration [i: 4, a: 2, b: 2, c: 1]:
 Condition not satisfied:
 
 i == 1
@@ -25,7 +25,7 @@ i == 1
 
 	at apackage.ASpec.a feature(script.groovy:15)
 
-Filtered iteration [i: 5, a: 1, b: 1, c: 3]:
+Filtered iteration [i: 5, a: 2, b: 2, c: 2]:
 Condition not satisfied:
 
 i == 1