Skip to content

Commit fb13584

Browse files
alex-spiesalex-spies
committed
Make tests respect Project's invariant
1 parent 1f82f93 commit fb13584

File tree

4 files changed

+36
-29
lines changed

4 files changed

+36
-29
lines changed

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/QueryPlanTests.java‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -77,19 +77,20 @@ public void testTransformWithExpressionTopLevelInCollection() throws Exception {
7777
}
7878

7979
public void testForEachWithExpressionTopLevel() throws Exception {
80-
Alias one = new Alias(EMPTY, "one", of(42));
80+
FieldAttribute one = fieldAttribute();
81+
Alias oneAliased = new Alias(EMPTY, "one", one);
8182
FieldAttribute two = fieldAttribute();
8283

83-
Project project = new Project(EMPTY, relation(), asList(one, two));
84+
Project project = new Project(EMPTY, relation(), asList(oneAliased, two));
8485

8586
List<Object> list = new ArrayList<>();
86-
project.forEachExpression(Literal.class, l -> {
87-
if (l.value().equals(42)) {
88-
list.add(l.value());
87+
project.forEachExpression(FieldAttribute.class, l -> {
88+
if (l.semanticEquals(one)) {
89+
list.add(l);
8990
}
9091
});
9192

92-
assertEquals(singletonList(one.child().fold(FoldContext.small())), list);
93+
assertEquals(singletonList(one), list);
9394
}
9495

9596
public void testForEachWithExpressionTree() throws Exception {
@@ -122,26 +123,30 @@ public void testForEachWithExpressionTopLevelInCollection() throws Exception {
122123
assertEquals(singletonList(one), list);
123124
}
124125

126+
// TODO: duplicate of testForEachWithExpressionTopLevel, let's remove it.
127+
// (Also a duplicate in the original ql package.)
125128
public void testForEachWithExpressionTreeInCollection() throws Exception {
126-
Alias one = new Alias(EMPTY, "one", of(42));
129+
FieldAttribute one = fieldAttribute();
130+
Alias oneAliased = new Alias(EMPTY, "one", one);
127131
FieldAttribute two = fieldAttribute();
128132

129-
Project project = new Project(EMPTY, relation(), asList(one, two));
133+
Project project = new Project(EMPTY, relation(), asList(oneAliased, two));
130134

131135
List<Object> list = new ArrayList<>();
132-
project.forEachExpression(Literal.class, l -> {
133-
if (l.value().equals(42)) {
134-
list.add(l.value());
136+
project.forEachExpression(FieldAttribute.class, l -> {
137+
if (l.semanticEquals(one)) {
138+
list.add(l);
135139
}
136140
});
137141

138-
assertEquals(singletonList(one.child().fold(FoldContext.small())), list);
142+
assertEquals(singletonList(one), list);
139143
}
140144

141145
public void testPlanExpressions() {
142-
Alias one = new Alias(EMPTY, "one", of(42));
146+
FieldAttribute one = fieldAttribute();
147+
Alias oneAliased = new Alias(EMPTY, "one", one);
143148
FieldAttribute two = fieldAttribute();
144-
Project project = new Project(EMPTY, relation(), asList(one, two));
149+
Project project = new Project(EMPTY, relation(), asList(oneAliased, two));
145150

146151
assertThat(Expressions.names(project.expressions()), contains("one", two.name()));
147152
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/logical/CommandLicenseTests.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,13 @@
1414
import org.elasticsearch.logging.Logger;
1515
import org.elasticsearch.test.ESTestCase;
1616
import org.elasticsearch.xpack.esql.LicenseAware;
17+
import org.elasticsearch.xpack.esql.core.tree.Node;
1718
import org.elasticsearch.xpack.esql.core.tree.Source;
1819
import org.elasticsearch.xpack.esql.expression.function.DocsV3Support;
1920
import org.elasticsearch.xpack.esql.parser.EsqlBaseParserVisitor;
2021
import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin;
2122
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
23+
import org.elasticsearch.xpack.esql.tree.EsqlNodeSubclassTests;
2224

2325
import java.lang.reflect.Constructor;
2426
import java.lang.reflect.InvocationTargetException;
@@ -179,9 +181,8 @@ private static LogicalPlan createInstance(Class<? extends LogicalPlan> clazz, Lo
179181
.min(Comparator.comparingInt(c -> c.getParameterTypes().length))
180182
.orElseThrow(() -> new IllegalArgumentException("No suitable constructor found for class " + clazz.getName()));
181183

182-
Class<?>[] paramTypes = constructor.getParameterTypes();
183-
Object[] args = new Object[paramTypes.length];
184-
args[0] = source;
184+
@SuppressWarnings("unchecked")
185+
Object[] args = EsqlNodeSubclassTests.ctorArgs((Constructor<? extends Node<?>>) constructor);
185186
args[1] = child;
186187
log.info("Creating instance of " + clazz.getName() + " with constructor: " + constructor);
187188
return (LogicalPlan) constructor.newInstance(args);

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/tree/EsqlNodeSubclassTests.java‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ public void testReplaceChildren() throws Exception {
312312
* {@code ctor} that make sense when {@code ctor}
313313
* builds subclasses of {@link Node}.
314314
*/
315-
private Object[] ctorArgs(Constructor<? extends Node<?>> ctor) throws Exception {
315+
public static Object[] ctorArgs(Constructor<? extends Node<?>> ctor) {
316316
Type[] argTypes = ctor.getGenericParameterTypes();
317317
Object[] args = new Object[argTypes.length];
318318
for (int i = 0; i < argTypes.length; i++) {
@@ -351,7 +351,7 @@ protected Object makeArg(Type argType) {
351351
* Make an argument to feed to the constructor for {@code toBuildClass}.
352352
*/
353353
@SuppressWarnings("unchecked")
354-
private Object makeArg(Class<? extends Node<?>> toBuildClass, Type argType) throws Exception {
354+
private static Object makeArg(Class<? extends Node<?>> toBuildClass, Type argType) throws Exception {
355355

356356
if (argType instanceof ParameterizedType pt) {
357357
if (pt.getRawType() == Map.class) {
@@ -525,31 +525,31 @@ public void accept(Page page) {
525525
}
526526
}
527527

528-
private List<?> makeList(Class<? extends Node<?>> toBuildClass, ParameterizedType listType) throws Exception {
528+
private static List<?> makeList(Class<? extends Node<?>> toBuildClass, ParameterizedType listType) throws Exception {
529529
return makeList(toBuildClass, listType, randomSizeForCollection(toBuildClass));
530530
}
531531

532-
private List<?> makeList(Class<? extends Node<?>> toBuildClass, ParameterizedType listType, int size) throws Exception {
532+
private static List<?> makeList(Class<? extends Node<?>> toBuildClass, ParameterizedType listType, int size) throws Exception {
533533
List<Object> list = new ArrayList<>();
534534
for (int i = 0; i < size; i++) {
535535
list.add(makeArg(toBuildClass, listType.getActualTypeArguments()[0]));
536536
}
537537
return list;
538538
}
539539

540-
private Set<?> makeSet(Class<? extends Node<?>> toBuildClass, ParameterizedType listType) throws Exception {
540+
private static Set<?> makeSet(Class<? extends Node<?>> toBuildClass, ParameterizedType listType) throws Exception {
541541
return makeSet(toBuildClass, listType, randomSizeForCollection(toBuildClass));
542542
}
543543

544-
private Set<?> makeSet(Class<? extends Node<?>> toBuildClass, ParameterizedType listType, int size) throws Exception {
544+
private static Set<?> makeSet(Class<? extends Node<?>> toBuildClass, ParameterizedType listType, int size) throws Exception {
545545
Set<Object> list = new HashSet<>();
546546
for (int i = 0; i < size; i++) {
547547
list.add(makeArg(toBuildClass, listType.getActualTypeArguments()[0]));
548548
}
549549
return list;
550550
}
551551

552-
private Object makeMap(Class<? extends Node<?>> toBuildClass, ParameterizedType pt) throws Exception {
552+
private static Object makeMap(Class<? extends Node<?>> toBuildClass, ParameterizedType pt) throws Exception {
553553
Map<Object, Object> map = new HashMap<>();
554554
int size = randomSizeForCollection(toBuildClass);
555555
while (map.size() < size) {
@@ -560,7 +560,7 @@ private Object makeMap(Class<? extends Node<?>> toBuildClass, ParameterizedType
560560
return map;
561561
}
562562

563-
private int randomSizeForCollection(Class<? extends Node<?>> toBuildClass) {
563+
private static int randomSizeForCollection(Class<? extends Node<?>> toBuildClass) {
564564
int minCollectionLength = 0;
565565
int maxCollectionLength = 10;
566566

@@ -584,7 +584,7 @@ private List<?> makeListOfSameSizeOtherThan(Type listType, List<?> original) thr
584584

585585
}
586586

587-
public <T extends Node<?>> T makeNode(Class<? extends T> nodeClass) throws Exception {
587+
public static <T extends Node<?>> T makeNode(Class<? extends T> nodeClass) throws Exception {
588588
if (Modifier.isAbstract(nodeClass.getModifiers())) {
589589
nodeClass = randomFrom(innerSubclassesOf(nodeClass));
590590
}
@@ -666,15 +666,15 @@ static <T> Constructor<T> longestCtor(Class<T> clazz) {
666666
return longest;
667667
}
668668

669-
private boolean hasAtLeastTwoChildren(Class<? extends Node<?>> toBuildClass) {
669+
private static boolean hasAtLeastTwoChildren(Class<? extends Node<?>> toBuildClass) {
670670
return CLASSES_WITH_MIN_TWO_CHILDREN.stream().anyMatch(toBuildClass::equals);
671671
}
672672

673673
static boolean isPlanNodeClass(Class<? extends Node<?>> toBuildClass) {
674674
return PhysicalPlan.class.isAssignableFrom(toBuildClass) || LogicalPlan.class.isAssignableFrom(toBuildClass);
675675
}
676676

677-
Expression randomResolvedExpression(Class<?> argClass) throws Exception {
677+
static Expression randomResolvedExpression(Class<?> argClass) throws Exception {
678678
assert Expression.class.isAssignableFrom(argClass);
679679
@SuppressWarnings("unchecked")
680680
Class<? extends Expression> asNodeSubclass = (Class<? extends Expression>) argClass;
@@ -740,7 +740,7 @@ public static <T> Set<Class<? extends T>> subclassesOf(Class<T> clazz) throws IO
740740
return subclassesOf(clazz, CLASSNAME_FILTER);
741741
}
742742

743-
private <T> Set<Class<? extends T>> innerSubclassesOf(Class<T> clazz) throws IOException {
743+
private static <T> Set<Class<? extends T>> innerSubclassesOf(Class<T> clazz) throws IOException {
744744
return subclassesOf(clazz, CLASSNAME_FILTER);
745745
}
746746

‎x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/plan/QueryPlanTests.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,7 @@ public void testForEachWithExpressionTopLevelInCollection() throws Exception {
121121
assertEquals(singletonList(one), list);
122122
}
123123

124+
// TODO: duplicate of testForEachWithExpressionTopLevel, let's remove it.
124125
public void testForEachWithExpressionTreeInCollection() throws Exception {
125126
Alias one = new Alias(EMPTY, "one", of(42));
126127
FieldAttribute two = fieldAttribute();

0 commit comments

Comments
 (0)