Fix some issues with custom format function #277
Conversation
| this.testLogging { | ||
| events("failed") | ||
| exceptionFormat = org.gradle.api.tasks.testing.logging.TestExceptionFormat.FULL | ||
| showExceptions = true | ||
| showCauses = true | ||
| showStackTraces = true | ||
| } |
There was a problem hiding this comment.
Added this bc it's much easier to see failing tests in the terminal than having to navigate out to a browser link to see the failing tests.
| } | ||
| | ||
| @Test | ||
| void testDouble() { |
There was a problem hiding this comment.
@jchadwick-buf this is what I meant by some nuance in testing. We could move these double tests to the conformance tests I guess, but not sure about things like testing the above thrown exception. Wdyt?
There was a problem hiding this comment.
Personally, I'm not overly concerned about handling all of the error cases identically as long as the error cases do fail as-expected, so I don't see this as a huge problem for adding tests to conformance per-se.
| final class Format { | ||
| private static final char[] HEX_ARRAY = "0123456789ABCDEF".toCharArray(); | ||
| private static final char[] LOWER_HEX_ARRAY = "0123456789abcdef".toCharArray(); | ||
| private static final DecimalFormat decimalFormat = new DecimalFormat("0.#########"); |
There was a problem hiding this comment.
DecimalFormat instances aren't thread safe - we should use a ThreadLocal to cache these (if we expect they'll be used a lot), add synchronization around them, or create them on demand:
Decimal formats are generally not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.
https://docs.oracle.com/javase/8/docs/api/java/text/DecimalFormat.html
| builder.append(val.value()); | ||
| } else if (type == TypeEnum.Bytes) { | ||
| formatBytes(builder, val); | ||
| builder.append(new String((byte[]) val.value(), StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Mainly a question on CEL in general but can we always assume bytes can be converted to a valid UTF-8 string? Is there some fallback where it would display invalid UTF-8 strings as hex or something like that?
There was a problem hiding this comment.
Our formatting is pretty inconsistent right now and we need to add more comprehensive tests to make sure it behaves the same. Right now, CEL-Go's built-in formatting function will throw a runtime error with invalid UTF-8, but our Java implementation will print placeholders �� when using %s and will just print hex digits when using %x. It's on the docket to unify all this behavior soon.
| } else if (type == TypeEnum.Bytes) { | ||
| formatBytes(builder, val); | ||
| builder.append(new String((byte[]) val.value(), StandardCharsets.UTF_8)); | ||
| } else if (type == TypeEnum.Int || type == TypeEnum.Uint || type == TypeEnum.Double) { |
There was a problem hiding this comment.
Shouldn't we only need to format float/double as decimals? It feels like for int/uint we could just output the string value.
smaye81 changed the title 
This fixes a few issues noticed from failing conformance tests:
durationadded to their formatting (along with escaped quotes)1.0should format as1, not1.0)In addition, this adds some additional unit tests for doubles to ensure the string formatting of doubles is adhering to the same behavior that CEL-Go uses.