Skip to content

Commit 256eb5c

Browse files
authored
Add conformance testing to string.format implementation (#293)
Apologies for the large PR. The logical way to split this up is for `Format.java` to be its own PR with everything else separate but it may just be easier to review this all at once. So, an easy way to review is: * `Format.java` for all the formatting fixes * Everything else that runs the conformance tests This adds conformance testing to our implementation of `string.format`. cel-java does not currently provide a `string.format` function so we implement our own and add it to the protovalidate-java CEL runtime. While formatting is not (yet) a part of the cel-spec, they do provide conformance test data for validating behavior. So, to ensure we are conformant, we use this test data and run it against our implementation. See the test data [here](https://github.com/google/cel-spec/blob/master/tests/simple/testdata/string_ext.textproto). Note that this file contains various sections for testing. However, we are only concerned with the `format` and `format_errors` sections. In addition, since we are only testing for formatting, we do not handle every `oneof` case or variation in the `SimpleTest` definition -- only expected results or evaluation errors. If others are added down the road in future versions of the spec, we can adjust these tests. There are few tests marked as skipped in `FormatTest` as these seem to be related to issues in cel-java's runtime (or in how we set up our environment) because the same issues happen when trying to use those expressions in protovalidate-java. Probably worth investigating or filing an issue for cel-java, but didn't want to hold up this PR.
1 parent 864c3f7 commit 256eb5c

File tree

7 files changed

+1868
-162
lines changed

7 files changed

+1868
-162
lines changed

build.gradle.kts

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,66 @@ tasks.register<Exec>("generateTestSourcesNoImports") {
9090
commandLine(buf.asPath, "generate", "--template", "src/test/resources/proto/buf.gen.noimports.yaml")
9191
}
9292

93+
tasks.register<Exec>("generateCelConformance") {
94+
dependsOn("generateCelConformanceTestTypes")
95+
description = "Generates CEL conformance code with buf generate for unit tests."
96+
commandLine(
97+
buf.asPath,
98+
"generate",
99+
"--template",
100+
"src/test/resources/proto/buf.gen.cel.yaml",
101+
"buf.build/google/cel-spec:${project.findProperty("cel.spec.version")}",
102+
"--exclude-path",
103+
"cel/expr/conformance/proto2",
104+
"--exclude-path",
105+
"cel/expr/conformance/proto3",
106+
)
107+
}
108+
109+
// The conformance tests use the Protobuf package path for tests that use these types.
110+
// i.e. cel.expr.conformance.proto3.TestAllTypes. But, if we use managed mode it adds 'com'
111+
// to the prefix. Additionally, we can't disable managed mode because the java_package option
112+
// specified in these proto files is "dev.cel.expr.conformance.proto3". So, to get around this,
113+
// we're generating these separately and specifying a java_package override of the package we need.
114+
tasks.register<Exec>("generateCelConformanceTestTypes") {
115+
dependsOn("exportProtovalidateModule")
116+
description = "Generates CEL conformance test types with buf generate for unit tests using a Java package override."
117+
commandLine(
118+
buf.asPath,
119+
"generate",
120+
"--template",
121+
"src/test/resources/proto/buf.gen.cel.testtypes.yaml",
122+
"buf.build/google/cel-spec:${project.findProperty("cel.spec.version")}",
123+
"--path",
124+
"cel/expr/conformance/proto3",
125+
)
126+
}
127+
128+
var getCelTestData =
129+
tasks.register<Exec>("getCelTestData") {
130+
val celVersion = project.findProperty("cel.spec.version")
131+
val fileUrl = "https://raw.githubusercontent.com/google/cel-spec/refs/tags/$celVersion/tests/simple/testdata/string_ext.textproto"
132+
val targetDir = File("${project.projectDir}/src/test/resources/testdata")
133+
val file = File(targetDir, "string_ext_$celVersion.textproto")
134+
135+
onlyIf {
136+
// Only run curl if file doesn't exist
137+
!file.exists()
138+
}
139+
doFirst {
140+
file.parentFile.mkdirs()
141+
commandLine(
142+
"curl",
143+
"-fsSL",
144+
"-o",
145+
file.absolutePath,
146+
fileUrl,
147+
)
148+
}
149+
}
150+
93151
tasks.register("generateTestSources") {
94-
dependsOn("generateTestSourcesImports", "generateTestSourcesNoImports")
152+
dependsOn("generateTestSourcesImports", "generateTestSourcesNoImports", "generateCelConformance")
95153
description = "Generates code with buf generate for unit tests"
96154
}
97155

@@ -206,6 +264,7 @@ allprojects {
206264
}
207265
}
208266
tasks.withType<Test>().configureEach {
267+
dependsOn(getCelTestData)
209268
useJUnitPlatform()
210269
this.testLogging {
211270
events("failed")

gradle.properties

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,6 @@ protovalidate.conformance.args = --strict_message --strict_error --expected_fail
66

77
# Argument to the license-header CLI
88
license-header.years = 2023-2024
9+
10+
# Version of the cel-spec that this implementation is conformant with
11+
cel.spec.version = v0.24.0

0 commit comments

Comments
 (0)