Add conformance testing to string.format implementation #293
Conversation
| // i.e. cel.expr.conformance.proto3.TestAllTypes. But, if we use managed mode it adds 'com' | ||
| // to the prefix. Additionally, we can't disable managed mode because the java_package option | ||
| // specified in these proto files is "dev.cel.expr.conformance.proto3". So, to get around this, | ||
| // we're generating these separately and specifying a java_package override of the package we need. |
There was a problem hiding this comment.
We could just use one buf.gen.yaml for both cel generation steps, but then we'd have to specify the conformance version there also when we generate. Then we'd have the version in a 3rd place, which doesn't seem ideal.
Also, if there is an easier way to override the java_package with a blank value, that would also work here but I don't think there is.
| } | ||
| return val.value().toString(); | ||
| case Bytes: | ||
| return new String((byte[]) val.value(), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
What does this do if the value is invalid UTF-8 here?
There was a problem hiding this comment.
Talked offline, but in the interest of visibility:
Currently this will print a placeholder �, but the strings.md doc on the cel-spec site specifies:
The value is formatted as if string(value) was performed and any invalid UTF-8 sequences are replaced with \ufffd. Multiple adjacent invalid UTF-8 sequences must be replaced with a single \ufffd.
So, this is currently non-conformant, but there are no actual conformance tests yet to cover this. Will implement this in a follow-up PR (with separate unit tests) if that's cool.
| while (iter.hasNext().booleanValue()) { | ||
| Val v = iter.next(); | ||
| builder.append(formatString(v)); | ||
| if (index != val.size().intValue() - 1) { |
There was a problem hiding this comment.
nit: Could use iter.hasNext() here instead of keeping track of an index.
| env = Env.newEnv(Library.Lib(new ValidateLibrary())); | ||
|
|
||
| } catch (InvalidPathException | IOException e) { | ||
| fail(e); |
There was a problem hiding this comment.
I'd just add a throws Exception to the setUp method which will automatically fail the test.
2 participants
Apologies for the large PR. The logical way to split this up is for
Format.javato 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.javafor all the formatting fixesThis adds conformance testing to our implementation of
string.format. cel-java does not currently provide astring.formatfunction 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. Note that this file contains various sections for testing. However, we are only concerned with the
formatandformat_errorssections. In addition, since we are only testing for formatting, we do not handle everyoneofcase or variation in theSimpleTestdefinition -- 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
FormatTestas 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.