Skip to content

Commit 4f9a3a8

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Add parsed-only evaluation coverage to Comprehensions Extensions
Includes a fix to preserve first encountered error message for comprehensions PiperOrigin-RevId: 900264092
1 parent 2203ac8 commit 4f9a3a8

File tree

5 files changed

+89
-77
lines changed

5 files changed

+89
-77
lines changed

‎extensions/src/test/java/dev/cel/extensions/CelComprehensionsExtensionsTest.java‎

Lines changed: 79 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,15 @@
1515
package dev.cel.extensions;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18+
import static com.google.common.truth.Truth.assertWithMessage;
1819
import static org.junit.Assert.assertThrows;
1920

21+
import com.google.common.base.Throwables;
22+
import com.google.common.collect.ImmutableMap;
2023
import com.google.testing.junit.testparameterinjector.TestParameter;
2124
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
2225
import com.google.testing.junit.testparameterinjector.TestParameters;
26+
import dev.cel.bundle.Cel;
2327
import dev.cel.common.CelAbstractSyntaxTree;
2428
import dev.cel.common.CelFunctionDecl;
2529
import dev.cel.common.CelOptions;
@@ -28,15 +32,15 @@
2832
import dev.cel.common.exceptions.CelIndexOutOfBoundsException;
2933
import dev.cel.common.types.SimpleType;
3034
import dev.cel.common.types.TypeParamType;
31-
import dev.cel.compiler.CelCompiler;
32-
import dev.cel.compiler.CelCompilerFactory;
3335
import dev.cel.parser.CelMacro;
3436
import dev.cel.parser.CelStandardMacro;
3537
import dev.cel.parser.CelUnparser;
3638
import dev.cel.parser.CelUnparserFactory;
3739
import dev.cel.runtime.CelEvaluationException;
38-
import dev.cel.runtime.CelRuntime;
39-
import dev.cel.runtime.CelRuntimeFactory;
40+
import dev.cel.testing.CelRuntimeFlavor;
41+
import java.util.Map;
42+
import org.junit.Assume;
43+
import org.junit.Before;
4044
import org.junit.Test;
4145
import org.junit.runner.RunWith;
4246

@@ -46,26 +50,35 @@ public class CelComprehensionsExtensionsTest {
4650

4751
private static final CelOptions CEL_OPTIONS =
4852
CelOptions.current()
53+
.enableHeterogeneousNumericComparisons(true)
4954
// Enable macro call population for unparsing
5055
.populateMacroCalls(true)
5156
.build();
5257

53-
private static final CelCompiler CEL_COMPILER =
54-
CelCompilerFactory.standardCelCompilerBuilder()
55-
.setOptions(CEL_OPTIONS)
56-
.setStandardMacros(CelStandardMacro.STANDARD_MACROS)
57-
.addLibraries(CelExtensions.comprehensions())
58-
.addLibraries(CelExtensions.lists())
59-
.addLibraries(CelExtensions.strings())
60-
.addLibraries(CelOptionalLibrary.INSTANCE, CelExtensions.bindings())
61-
.build();
62-
private static final CelRuntime CEL_RUNTIME =
63-
CelRuntimeFactory.standardCelRuntimeBuilder()
64-
.addLibraries(CelOptionalLibrary.INSTANCE)
65-
.addLibraries(CelExtensions.lists())
66-
.addLibraries(CelExtensions.strings())
67-
.addLibraries(CelExtensions.comprehensions())
68-
.build();
58+
@TestParameter public CelRuntimeFlavor runtimeFlavor;
59+
@TestParameter public boolean isParseOnly;
60+
61+
private Cel cel;
62+
63+
@Before
64+
public void setUp() {
65+
// Legacy runtime does not support parsed-only evaluation mode.
66+
Assume.assumeFalse(runtimeFlavor.equals(CelRuntimeFlavor.LEGACY) && isParseOnly);
67+
this.cel =
68+
runtimeFlavor
69+
.builder()
70+
.setOptions(CEL_OPTIONS)
71+
.setStandardMacros(CelStandardMacro.STANDARD_MACROS)
72+
.addCompilerLibraries(CelExtensions.comprehensions())
73+
.addCompilerLibraries(CelExtensions.lists())
74+
.addCompilerLibraries(CelExtensions.strings())
75+
.addCompilerLibraries(CelOptionalLibrary.INSTANCE, CelExtensions.bindings())
76+
.addRuntimeLibraries(CelOptionalLibrary.INSTANCE)
77+
.addRuntimeLibraries(CelExtensions.lists())
78+
.addRuntimeLibraries(CelExtensions.strings())
79+
.addRuntimeLibraries(CelExtensions.comprehensions())
80+
.build();
81+
}
6982

7083
private static final CelUnparser UNPARSER = CelUnparserFactory.newUnparser();
7184

@@ -101,11 +114,7 @@ public void allMacro_twoVarComprehension_success(
101114
})
102115
String expr)
103116
throws Exception {
104-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst();
105-
106-
Object result = CEL_RUNTIME.createProgram(ast).eval();
107-
108-
assertThat(result).isEqualTo(true);
117+
assertThat(eval(expr)).isEqualTo(true);
109118
}
110119

111120
@Test
@@ -127,11 +136,7 @@ public void existsMacro_twoVarComprehension_success(
127136
})
128137
String expr)
129138
throws Exception {
130-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst();
131-
132-
Object result = CEL_RUNTIME.createProgram(ast).eval();
133-
134-
assertThat(result).isEqualTo(true);
139+
assertThat(eval(expr)).isEqualTo(true);
135140
}
136141

137142
@Test
@@ -156,11 +161,7 @@ public void exists_oneMacro_twoVarComprehension_success(
156161
})
157162
String expr)
158163
throws Exception {
159-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst();
160-
161-
Object result = CEL_RUNTIME.createProgram(ast).eval();
162-
163-
assertThat(result).isEqualTo(true);
164+
assertThat(eval(expr)).isEqualTo(true);
164165
}
165166

166167
@Test
@@ -182,11 +183,7 @@ public void transformListMacro_twoVarComprehension_success(
182183
})
183184
String expr)
184185
throws Exception {
185-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst();
186-
187-
Object result = CEL_RUNTIME.createProgram(ast).eval();
188-
189-
assertThat(result).isEqualTo(true);
186+
assertThat(eval(expr)).isEqualTo(true);
190187
}
191188

192189
@Test
@@ -210,11 +207,7 @@ public void transformMapMacro_twoVarComprehension_success(
210207
})
211208
String expr)
212209
throws Exception {
213-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst();
214-
215-
Object result = CEL_RUNTIME.createProgram(ast).eval();
216-
217-
assertThat(result).isEqualTo(true);
210+
assertThat(eval(expr)).isEqualTo(true);
218211
}
219212

220213
@Test
@@ -238,24 +231,22 @@ public void transformMapEntryMacro_twoVarComprehension_success(
238231
})
239232
String expr)
240233
throws Exception {
241-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst();
242-
243-
Object result = CEL_RUNTIME.createProgram(ast).eval();
244-
245-
assertThat(result).isEqualTo(true);
234+
assertThat(eval(expr)).isEqualTo(true);
246235
}
247236

248237
@Test
249238
public void comprehension_onTypeParam_success() throws Exception {
250-
CelCompiler celCompiler =
251-
CelCompilerFactory.standardCelCompilerBuilder()
239+
Assume.assumeFalse(isParseOnly);
240+
Cel customCel =
241+
runtimeFlavor
242+
.builder()
252243
.setOptions(CEL_OPTIONS)
253244
.setStandardMacros(CelStandardMacro.STANDARD_MACROS)
254-
.addLibraries(CelExtensions.comprehensions())
245+
.addCompilerLibraries(CelExtensions.comprehensions())
255246
.addVar("items", TypeParamType.create("T"))
256247
.build();
257248

258-
CelAbstractSyntaxTree ast = celCompiler.compile("items.all(i, v, v > 0)").getAst();
249+
CelAbstractSyntaxTree ast = customCel.compile("items.all(i, v, v > 0)").getAst();
259250

260251
assertThat(ast.getResultType()).isEqualTo(SimpleType.BOOL);
261252
}
@@ -275,7 +266,7 @@ public void unparseAST_twoVarComprehension(
275266
})
276267
String expr)
277268
throws Exception {
278-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst();
269+
CelAbstractSyntaxTree ast = isParseOnly ? cel.parse(expr).getAst() : cel.compile(expr).getAst();
279270
String unparsed = UNPARSER.unparse(ast);
280271
assertThat(unparsed).isEqualTo(expr);
281272
}
@@ -318,8 +309,9 @@ public void unparseAST_twoVarComprehension(
318309
"{expr: \"{'hello': 'world', 'greetings': 'tacocat'}.transformMapEntry(k, v, []) == {}\","
319310
+ " err: 'no matching overload'}")
320311
public void twoVarComprehension_compilerErrors(String expr, String err) throws Exception {
312+
Assume.assumeFalse(isParseOnly);
321313
CelValidationException e =
322-
assertThrows(CelValidationException.class, () -> CEL_COMPILER.compile(expr).getAst());
314+
assertThrows(CelValidationException.class, () -> cel.compile(expr).getAst());
323315

324316
assertThat(e).hasMessageThat().contains(err);
325317
}
@@ -339,34 +331,50 @@ public void twoVarComprehension_compilerErrors(String expr, String err) throws E
339331
+ " '2.0' already exists\"}")
340332
public void twoVarComprehension_keyCollision_runtimeError(String expr, String err)
341333
throws Exception {
342-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst();
343-
344-
CelEvaluationException e =
345-
assertThrows(CelEvaluationException.class, () -> CEL_RUNTIME.createProgram(ast).eval());
346-
347-
assertThat(e).hasCauseThat().isInstanceOf(IllegalArgumentException.class);
348-
assertThat(e).hasCauseThat().hasMessageThat().contains(err);
334+
// Planner does not allow decimals for map keys
335+
Assume.assumeFalse(runtimeFlavor.equals(CelRuntimeFlavor.PLANNER) && expr.contains("2.0"));
336+
337+
CelEvaluationException e = assertThrows(CelEvaluationException.class, () -> eval(expr));
338+
Throwable cause =
339+
Throwables.getCausalChain(e).stream()
340+
.filter(IllegalArgumentException.class::isInstance)
341+
.filter(t -> t.getMessage() != null && t.getMessage().contains(err))
342+
.findFirst()
343+
.orElse(null);
344+
345+
assertWithMessage(
346+
"Expected IllegalArgumentException with message containing '%s' in cause chain", err)
347+
.that(cause)
348+
.isNotNull();
349349
}
350350

351351
@Test
352352
public void twoVarComprehension_arithmeticException_runtimeError() throws Exception {
353-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile("[0].all(i, k, i/k < k)").getAst();
354-
355353
CelEvaluationException e =
356-
assertThrows(CelEvaluationException.class, () -> CEL_RUNTIME.createProgram(ast).eval());
357-
354+
assertThrows(CelEvaluationException.class, () -> eval("[0].all(i, k, i/k < k)"));
358355
assertThat(e).hasCauseThat().isInstanceOf(CelDivideByZeroException.class);
359356
assertThat(e).hasCauseThat().hasMessageThat().contains("/ by zero");
360357
}
361358

362359
@Test
363360
public void twoVarComprehension_outOfBounds_runtimeError() throws Exception {
364-
CelAbstractSyntaxTree ast = CEL_COMPILER.compile("[1, 2].exists(i, v, [0][v] > 0)").getAst();
365-
366361
CelEvaluationException e =
367-
assertThrows(CelEvaluationException.class, () -> CEL_RUNTIME.createProgram(ast).eval());
368-
362+
assertThrows(CelEvaluationException.class, () -> eval("[1, 2].exists(i, v, [0][v] > 0)"));
369363
assertThat(e).hasCauseThat().isInstanceOf(CelIndexOutOfBoundsException.class);
370364
assertThat(e).hasCauseThat().hasMessageThat().contains("Index out of bounds: 1");
371365
}
366+
367+
private Object eval(String expression) throws Exception {
368+
return eval(this.cel, expression, ImmutableMap.of());
369+
}
370+
371+
private Object eval(Cel cel, String expression, Map<String, ?> variables) throws Exception {
372+
CelAbstractSyntaxTree ast;
373+
if (isParseOnly) {
374+
ast = cel.parse(expression).getAst();
375+
} else {
376+
ast = cel.compile(expression).getAst();
377+
}
378+
return cel.createProgram(ast).eval(variables);
379+
}
372380
}

‎extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -334,8 +334,6 @@ private static Cel setupEnv(CelBuilder celBuilder) {
334334
.build();
335335
}
336336

337-
338-
339337
private Object eval(Cel cel, String expr) throws Exception {
340338
return eval(cel, expr, ImmutableMap.of());
341339
}

‎runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,10 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) {
3838
return false;
3939
}
4040
} else if (argVal instanceof ErrorValue) {
41-
errorValue = (ErrorValue) argVal;
41+
// Preserve the first encountered error instead of overwriting it with subsequent errors.
42+
if (errorValue == null) {
43+
errorValue = (ErrorValue) argVal;
44+
}
4245
} else if (argVal instanceof AccumulatedUnknowns) {
4346
unknowns = AccumulatedUnknowns.maybeMerge(unknowns, argVal);
4447
} else {

‎runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,10 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) {
3838
return true;
3939
}
4040
} else if (argVal instanceof ErrorValue) {
41-
errorValue = (ErrorValue) argVal;
41+
// Preserve the first encountered error instead of overwriting it with subsequent errors.
42+
if (errorValue == null) {
43+
errorValue = (ErrorValue) argVal;
44+
}
4245
} else if (argVal instanceof AccumulatedUnknowns) {
4346
unknowns = AccumulatedUnknowns.maybeMerge(unknowns, argVal);
4447
} else {

‎runtime/src/test/resources/planner_unknownResultSet_errors.baseline‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ single_timestamp {
3232
seconds: 15
3333
}
3434
, unknown_attributes=[x.single_int32]}
35-
error: evaluation error at test_location:89: Text 'another bad timestamp string' could not be parsed at index 0
35+
error: evaluation error at test_location:31: Text 'bad timestamp string' could not be parsed at index 0
3636
error_code: BAD_FORMAT
3737

3838
Source: x.single_int32 == 1 || x.single_timestamp <= timestamp("bad timestamp string")
@@ -69,7 +69,7 @@ single_timestamp {
6969
seconds: 15
7070
}
7171
, unknown_attributes=[x.single_int32]}
72-
error: evaluation error at test_location:89: Text 'another bad timestamp string' could not be parsed at index 0
72+
error: evaluation error at test_location:31: Text 'bad timestamp string' could not be parsed at index 0
7373
error_code: BAD_FORMAT
7474

7575
Source: x

0 commit comments

Comments
 (0)