-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Cartesian products of environments in MultiEnvTestEngine #7816
base: main
Are you sure you want to change the base?
Changes from 20 commits
7620b94
554dfab
e0a8dd3
db22c92
428728a
e004be1
5faf681
870d25a
5cb8b1b
e8131d4
67bc7f2
ab0fd2b
3df881b
db1a3a6
5104a20
732fc44
379935f
577685a
b9c229d
f4e11e4
a1abab1
a8ce700
d6fef14
95e8590
915fe05
3439176
1d6166e
fafab6d
67106da
c32b55b
4a8af55
c7799d0
50b3141
eea4d5d
3ff67f4
40169b4
b695dee
4a58d99
3febfbf
f0219e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
/* | ||
* Copyright (C) 2023 Dremio | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.projectnessie.junit.engine; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.concurrent.atomic.AtomicBoolean; | ||
import java.util.function.Predicate; | ||
import java.util.stream.Collectors; | ||
import org.junit.jupiter.engine.config.CachingJupiterConfiguration; | ||
import org.junit.jupiter.engine.config.JupiterConfiguration; | ||
import org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor; | ||
import org.junit.jupiter.engine.discovery.DiscoverySelectorResolver; | ||
import org.junit.platform.engine.ConfigurationParameters; | ||
import org.junit.platform.engine.EngineDiscoveryRequest; | ||
import org.junit.platform.engine.TestDescriptor; | ||
import org.junit.platform.engine.UniqueId; | ||
import org.junit.platform.engine.UniqueId.Segment; | ||
|
||
public class MultiEnvTestDescriptorTree { | ||
|
||
private static final boolean FAIL_ON_MISSING_ENVIRONMENTS = | ||
!Boolean.getBoolean("org.projectnessie.junit.engine.ignore-empty-environments"); | ||
|
||
private final TestDescriptor rootNode; | ||
private final ConfigurationParameters configurationParameters; | ||
private final AtomicBoolean foundAtLeastOneEnvironmentId = new AtomicBoolean(); | ||
private final List<String> processedExtensionNames = new ArrayList<>(); | ||
private List<TestDescriptor> latestLeafNodes; | ||
|
||
public MultiEnvTestDescriptorTree( | ||
TestDescriptor rootNode, ConfigurationParameters configurationParameters) { | ||
this.rootNode = rootNode; | ||
this.latestLeafNodes = Collections.singletonList(rootNode); | ||
this.configurationParameters = configurationParameters; | ||
} | ||
|
||
public void appendDescriptorsForExtension(MultiEnvTestExtension multiEnvTestExtension) { | ||
processedExtensionNames.add(multiEnvTestExtension.getClass().getSimpleName()); | ||
|
||
List<TestDescriptor> newLeafNodes = new ArrayList<>(); | ||
for (TestDescriptor parentNode : latestLeafNodes) { | ||
for (String environmentId : | ||
multiEnvTestExtension.allEnvironmentIds(configurationParameters)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
foundAtLeastOneEnvironmentId.set(true); | ||
UniqueId newNodeId = | ||
parentNode.getUniqueId().append(multiEnvTestExtension.segmentType(), environmentId); | ||
MultiEnvTestDescriptor newNode = new MultiEnvTestDescriptor(newNodeId, environmentId); | ||
parentNode.addChild(newNode); | ||
newLeafNodes.add(newNode); | ||
} | ||
} | ||
|
||
latestLeafNodes = newLeafNodes; | ||
} | ||
|
||
public void addOriginalChildrenToFinishedTree(EngineDiscoveryRequest discoveryRequest) { | ||
final String rootNodeSegmentValue = rootNode.getUniqueId().getSegments().get(0).getValue(); | ||
|
||
for (TestDescriptor leafNode : latestLeafNodes) { | ||
String environmentNames = | ||
leafNode.getUniqueId().getSegments().stream() | ||
.map(Segment::getValue) | ||
.filter(Predicate.not(rootNodeSegmentValue::equals)) | ||
.collect(Collectors.joining(",")); | ||
JupiterConfiguration nodeConfiguration = | ||
new CachingJupiterConfiguration( | ||
new MultiEnvJupiterConfiguration(configurationParameters, environmentNames)); | ||
JupiterEngineDescriptor discoverResult = | ||
new JupiterEngineDescriptor(leafNode.getUniqueId(), nodeConfiguration); | ||
new DiscoverySelectorResolver().resolveSelectors(discoveryRequest, discoverResult); | ||
for (TestDescriptor childWithProperId : discoverResult.getChildren()) { | ||
leafNode.addChild(childWithProperId); | ||
} | ||
} | ||
} | ||
|
||
public TestDescriptor getRootNode() { | ||
if (FAIL_ON_MISSING_ENVIRONMENTS | ||
&& !processedExtensionNames.isEmpty() | ||
&& !foundAtLeastOneEnvironmentId.get()) { | ||
throw new IllegalStateException( | ||
String.format( | ||
"%s was enabled, but test extensions %s did not discover any environment IDs.", | ||
MultiEnvTestEngine.class.getSimpleName(), | ||
Arrays.toString(processedExtensionNames.toArray()))); | ||
} | ||
|
||
return rootNode; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rename? The
String
is opaque in this context, does not convey anything about names, IMHO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 tosuffix
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:
There are a number of concepts that need names:
If we go from the name here, it seems #4 is meant to be "environment"
Possible names to make this work:
e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 oldenvironment
works just as well :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
- MaybeCategory
? (in the statistical sense)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
My vote at the moment is "dimension", "type", and "value":
For now, I've reverted these back to
environment
and will see what the rename would look like in practice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 consideredcurrentMultiEnvSegmentTypes/Values
to clarify this. Now that it iscurrentDimensionTypes/Values
, the ambiguity is also resolved.