Support Cartesian products of environments in MultiEnvTestEngine by colan-dremio · Pull Request #7816 · projectnessie/nessie

colan-dremio · 2023-12-06T20:04:23Z

Currently, MultiEnvTestEngine only supports a single MultiEnvTestExtension at a time. This adds support for multiple MultiEnvTestExtensions. It will perform a Cartesian product of the registered environment IDs.

For example, it would become possible to test multiple Nessie client versions with multiple Nessie server versions through a combination of OlderNessieClientsExtension and OlderNessieServersExtension. With nessie.versions property set to 1,2, the following test environments would run:

[client:1][server:1]
[client:1][server:2]
[client:2][server:1]
[client:2][server:2]

This is also enables downstream projects to use their own implementations of MultiEnvTestExtension. For example, a downstream project could test multiple Nessie versions against multiple types of object storage.

Future work: Does not yet support complex @Nested tests. Two tests are added but @Disabled for future development, see onlyNestedTest and cartesianNestedTest.

...t/java/org/projectnessie/tools/compatibility/internal/TestNessieCompatibilityExtensions.java

...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java

...env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestDescriptorTree.java

dimas-b

Thanks for your contribution @colan-dremio !

This is just a preliminary quick review from my side :)

dimas-b · 2023-12-06T23:49:57Z

...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java

+ * </ul>
+ * will result in the following IDs:
+ * <ul>
+ *   <li>[engine:nessie-multi-env][segmentA:1][segmentB:1][segmentC:1]</li>


I believe MultiEnvTestFilter needs to be updated to check whether all multi-env ID segments are recognised... Currently it checks that any match.

I don't mind doing this, but I'm unsure if it is necessary. This would couple the cartesian product logic to the filter. The MultiEnvTestEngine is responsible for expanding the environments. Perhaps other types of expansion would be desirable in the future? From the filter's perspective, it seems reasonable to include a test as long as we see at least one correct segment type. This shouldn't happen in practice, so either way is fine with me.

At the time, I was not correct above about the expansion logic, but now that it is fixed it should hold.

...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java

...-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvExtensionRegistry.java

...ulti-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestExtension.java

...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java

# Conflicts: # compatibility/common/src/test/java/org/projectnessie/tools/compatibility/internal/TestNessieCompatibilityExtensions.java

# Conflicts: # testing/multi-env-test-engine/src/test/java/org/projectnessie/junit/engine/TestMultiEnvTestEngine.java

colan-dremio · 2024-05-16T00:41:35Z

Note: I am developing on a Mac and could not validate TestNessieCompatibilityExtensions::upgrade or TestNessieCompatibilityExtensions::tooManyExtensions locally.

dimas-b · 2024-05-16T23:44:14Z

...v-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvDisplayNameGenerator.java

+  public MultiEnvDisplayNameGenerator(DisplayNameGenerator delegate, String environmentNames) {
    this.delegate = delegate;
-    this.environment = environment;
+    this.environmentNames = environmentNames;


why rename? The String is opaque in this context, does not convey anything about names, IMHO.

On review, I meant environmentIds not names, updated accordingly. The goal was to give a bit of clarity into what the value holds. If we want to treat it as opaque, I can rename to suffix instead.

More generally, I've found that there is some ambiguity in naming. This is a multiple environment test engine, what is an environment? Take the example OlderNessieServersExtension (S1, S2) and OlderNessieClientsExtension (C1) which will run:

S1, C1

S2, C1

There are a number of concepts that need names:

Each multienv extension (e.g. OlderNessieServersExtension)

Each segement type (e.g. nessie-server-version)

Each "environment id" within an extension (e.g. S1 and S2 individually)

Each unique combination of environment ids on a test execution (e.g. [S1, C1])

If we go from the name here, it seems #4 is meant to be "environment"

Possible names to make this work:

Dimension

Dimension type

Dimension element

Environment

e.g.

public interface MultiEnvTestExtension extends Extension { String dimensionType(); List<String> allDimensionElements(ConfigurationParameters configuration); default int dimensionPriority() { return 0; } } Thoughts?

This particular class is meant to make multi-env testa distinguishable in tools that do not understand Junit5 UniqueId (i.e. those that deal with test class/method names only). environmentIds works in this case, but I still think the rename is unnecessary in this case as the old environment works just as well :)

1. Dimention - SGTM (but let's wait a bit with renaming :) )
2. Dimention type - I prefer segment (current) as it related to JUnit5 ID segments.
3. Dimention elements - Maybe Category? (in the statistical sense)

Keeping environment as # 4 is fine with me. I don't think "category" should be used for elements, "category" is synonymous with dimension type / segment type. Also, I don't think we should use "segment" alone because a UniqueId segment is the pair of segment type + segment value.

It looks like we should choose

One of (dimension | segment | category) for # 2

Whether or not to use "type" as a suffix for # 2. I'd avoid "segment" alone, but could see "category" or "category type" working fine.

One of (value | element | ID) for # 3

My vote at the moment is "dimension", "type", and "value":

public @interface MultiEnvDimensionType {} public interface MultiEnvTestExtension extends Extension { List<String> allDimensionValues(ConfigurationParameters configuration); int segmentPriority() }

For now, I've reverted these back to environment and will see what the rename would look like in practice.

Proposed naming: colan-dremio@e4625b7

There are some subtle advantages here. For example in this block, it can be ambiguous about whether currentSegmentTypes/Values is referring to MultiEnv segments or all JUnit segments (engine, class, nested-class, method too). I had considered currentMultiEnvSegmentTypes/Values to clarify this. Now that it is currentDimensionTypes/Values, the ambiguity is also resolved.

...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java

...lti-env-test-engine/src/test/java/org/projectnessie/junit/engine/TestMultiEnvTestEngine.java

...v-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvJupiterConfiguration.java

dimas-b · 2024-05-17T00:08:51Z

...env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestDescriptorTree.java

+    List<TestDescriptor> newLeafNodes = new ArrayList<>();
+    for (TestDescriptor parentNode : latestLeafNodes) {
+      for (String environmentId :
+          multiEnvTestExtension.allEnvironmentIds(configurationParameters)) {


TBH, I find this method of constructing a Cartesian product of environments a bit obscure. Why not simply have nested loops in one method (without calling out to this helper class)?

I've moved the tree construction logic directly into the engine implementation as part of fixing the updated cartesian test. The main annoyance is that the tree of nodes (represented by recursive child references) sits independently of the UniqueIds of those nodes. My new implementation still maintains a cache of known nodes to keep lookup simple, but should be simpler overall.

...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java

# Conflicts: # testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestDescriptorTree.java

dimas-b

Only a partial review this time, but I think we're getting close to the end :)

...t/java/org/projectnessie/tools/compatibility/internal/TestNessieCompatibilityExtensions.java

dimas-b · 2024-05-22T22:44:33Z

...ulti-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestExtension.java

 * Interface for JUnit5 test extensions that require running the same suite of tests in multiple
 * executions environments. For example, running the same tests for multiple versions of a Nessie
- * Client.
+ * Client.<br>


Maybe just <p>? cf.

nessie/api/client/src/main/java/org/projectnessie/client/builder/StreamingUtil.java

Line 32 in 81b34b6

* <p>Use the functions that return {@link Stream}s in the builders that implement {@link

dimas-b · 2024-05-22T23:28:11Z

...-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvExtensionRegistry.java

-                .filter(MultiEnvTestExtension.class::isAssignableFrom)
-                .flatMap(registry::stream);
-    return r;
+    return findMultiEnvTestExtensionsOn(testClass).flatMap(registry::stream);


Maybe use it in registerExtensions too?

dimas-b · 2024-05-22T23:29:03Z

...-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvExtensionRegistry.java

@@ -51,19 +51,6 @@ public Stream<MultiEnvTestExtension> stream() {
  }

  public Stream<? extends MultiEnvTestExtension> stream(Class<?> testClass) {


stream() (no args) is unused now.

Weird, my IDE seems to be confused about this one. Removed.

dimas-b · 2024-05-22T23:37:31Z

...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java

+  }
+
+  /** Immutable key of segment types for the intermediate cartesian product tree. */
+  private static class SegmentTypes {


Why not @Value.Immutable? It would be possible to add default helper methods for .root() and .append(String child) even if SegmentTypes were an interface.

dimas-b · 2024-05-22T23:51:32Z

...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java

+                putTestIntoParent(
+                    testDescriptor, nodeAtCurrentPosition, environmentNames, discoveryRequest);


I think it might still be preferable to use the old resolveSelectors(...) method... How does the new code behave when (e.g. IDE) selects only one test method for execution? For example, run the whole test suite, but then select only one test in one particular environment for debugging in IntelliJ.

In this PR in my env. that will run the test method in all environments, before the PR, only in one env (as it should).

Added this back. It had some interesting side effects where resolution would discover children that do not apply. With some post-filtering, it looks like this works better.

Added tests to cover these scenarios.

…esn't select too many things

colan-dremio added 3 commits December 4, 2023 18:43

Support cartesian products of environments in MultiEnvTestEngine

7620b94

Update tests to assert on entire UniqueIds

554dfab

Merge remote-tracking branch 'upstream/main' into cartesian-multienv

e0a8dd3

colan-dremio marked this pull request as draft December 6, 2023 20:04

colan-dremio commented Dec 6, 2023

View reviewed changes

colan-dremio mentioned this pull request Dec 6, 2023

Increase access level for MultiEnvTestEngine helpers #7779

Closed

dimas-b reviewed Dec 7, 2023

View reviewed changes

colan-dremio added 7 commits December 8, 2023 09:38

Merge remote-tracking branch 'upstream/main' into cartesian-multienv

db22c92

Remove second registry

428728a

Support ordering of extensions

e004be1

Clear registry between tests

5faf681

Use separate registry in MultiEnvTestFilter

870d25a

Re-add tooManyExtensions()

5cb8b1b

Clear filter registry between tests too

e8131d4

dimas-b reviewed Dec 12, 2023

View reviewed changes

...-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvExtensionRegistry.java Outdated Show resolved Hide resolved

...ulti-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestExtension.java Outdated Show resolved Hide resolved

dimas-b reviewed Dec 12, 2023

View reviewed changes

...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java Outdated Show resolved Hide resolved

colan-dremio added 9 commits May 14, 2024 10:15

Merge remote-tracking branch 'upstream/main' into cartesian-multienv

67bc7f2

Clear registries between Nessie compat tests

ab0fd2b

# Conflicts: # compatibility/common/src/test/java/org/projectnessie/tools/compatibility/internal/TestNessieCompatibilityExtensions.java

Add missing assert

3df881b

# Conflicts: # testing/multi-env-test-engine/src/test/java/org/projectnessie/junit/engine/TestMultiEnvTestEngine.java

spotless apply

db1a3a6

Call clear from registry constructor

5104a20

Rename getOrder to segmentPriority

732fc44

clarify filter logic

379935f

Add environment names to test display names

577685a

update segmentPriority docs

b9c229d

colan-dremio marked this pull request as ready for review May 16, 2024 00:39

Reorder extensions to better exercise ordering test

f4e11e4

dimas-b reviewed May 17, 2024

View reviewed changes

Expand cartesian product test

a1abab1

colan-dremio added 8 commits May 21, 2024 20:31

Do calculation of partial cartesian product up front in the engine

a8ce700

environmentNames -> environmentIds

d6fef14

# Conflicts: # testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestDescriptorTree.java

Remove filter's dependence on registry

95e8590

Remove unnecessary field

915fe05

spotlessApply

3439176

Merge remote-tracking branch 'upstream/main' into cartesian-multienv

1d6166e

environmentIds -> environment

fafab6d

Add support for TestTemplateTestDescriptor

67106da

dimas-b reviewed May 23, 2024

View reviewed changes

colan-dremio added 11 commits May 22, 2024 18:04

paragraph

c32b55b

immutables

4a8af55

unused method

c7799d0

UniqueId selection tests

50b3141

Switch back to DiscoverySelectorResolver, add post filtering so it do…

eea4d5d

…esn't select too many things

Add complex nesting tests, some failing

3ff67f4

Re-use nested find in registry

40169b4

fixup

b695dee

Merge remote-tracking branch 'upstream/main' into cartesian-multienv

4a58d99

remove duplicate annotations

3febfbf

update new extension

f0219e1

		@@ -51,19 +51,6 @@ public Stream<MultiEnvTestExtension> stream() {
		}

		public Stream<? extends MultiEnvTestExtension> stream(Class<?> testClass) {

		putTestIntoParent(
		testDescriptor, nodeAtCurrentPosition, environmentNames, discoveryRequest);

Conversation

colan-dremio commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimas-b left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colan-dremio commented May 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

colan-dremio commented Dec 6, 2023 •

edited

Loading

dimas-b left a comment •

edited

Loading

dimas-b May 22, 2024 •

edited

Loading