Pure-Java conformance tests - #941
Conversation
The original Bazel-driven build in CEL-Java to run conformance-tests was leveraging the `simple_test` convenience binary to run tests. That binary no longer exists for CEL-Spec 0.25.2 and was replaced with a Java client in the CEL-Spec version bump PR. This change goes further and removes the Bazel build entirely and replaces the client/server split, which was needed for the `simple_test` tool, with JUnit test-factory.
XN137
left a comment
XN137
left a comment
There was a problem hiding this comment.
+1 assuming we dont want to support concurrent execution
| "wrappers/timestamp/to_json", | ||
| "wrappers/empty/to_json"); | ||
|
|
||
| private static final Set<String> matchedSkips = new LinkedHashSet<>(); |
There was a problem hiding this comment.
sonnet finding:
AtomicInteger is used correctly for the counters, but matchedSkips is an unsynchronized LinkedHashSet. JUnit 5 @testfactory tests inside a DynamicContainer can execute concurrently under @execution(CONCURRENT). Even without that
annotation, test engines may parallelize by default in some configurations. Concurrent add() calls on a LinkedHashSet produce undefined behavior. Should be:
private static final Set matchedSkips = Collections.synchronizedSet(new LinkedHashSet<>());
or just a ConcurrentHashMap.newKeySet() (which sacrifices insertion order but that's fine here since it's only used for membership tests).
| Stream<DynamicNode> simpleConformance() { | ||
| List<DynamicNode> files = new ArrayList<>(); | ||
| TEST_FILES.forEach(fileName -> files.add(dynamicContainer(fileName, fileTests(fileName)))); | ||
| files.add(dynamicTest("skip list matches testdata", this::assertAllSkipsMatched)); |
There was a problem hiding this comment.
sonnet finding:
This sentinel is appended to the file list and relies on JUnit executing it after all dynamic tests have populated matchedSkips. JUnit does not guarantee ordering of dynamic nodes under concurrent execution. If anything parallel is ever
enabled this will produce false positives. A safer approach would be to collect matchedSkips from the SkipList.parse() result and do the check structurally (compare SKIP_TESTS against the parsed testdata at startup rather than tracking
hits at runtime).
| @AfterAll | ||
| static void printSummary() { | ||
| System.out.printf( | ||
| "Conformance tests: %d total, %d passed, %d skipped%n", |
There was a problem hiding this comment.
i assume we confirmed we pass and skip the same amount as previously
There was a problem hiding this comment.
Yes - it'll be eventually be even more
|
|
||
| Optional CEL-Spec files such as extension libraries, optionals, and type | ||
| deduction are intentionally not enabled by default. Add those separately with | ||
| explicit skip reasoning. |
There was a problem hiding this comment.
is the last sentence a TODO ?
There was a problem hiding this comment.
Yes, scope to follow-up work.
2 participants
The original Bazel-driven build in CEL-Java to run conformance-tests was leveraging the
simple_testconvenience binary to run tests. That binary no longer exists for CEL-Spec 0.25.2 and was replaced with a Java client in the CEL-Spec version bump PR.This change goes further and removes the Bazel build entirely and replaces the client/server split, which was needed for the
simple_testtool, with JUnit test-factory.