Updates conformance comparison report and exception handling #1547
Conversation
test/partiql-tests-runner/src/test/kotlin/org/partiql/runner/test/TestRunner.kt
Show resolved
Hide resolved
| val actual = executor.execute(statement) | ||
| executor.toIon(actual) | ||
| } catch (t: Throwable) { | ||
| thrown = t |
There was a problem hiding this comment.
Not related to this PR, but since we started enabling the thrown errors to propagate through, I noticed that some of the errors are kinda non-descriptive.
For example, this test run in permissive mode https://github.com/partiql/partiql-tests/blob/9aa7694eb12b4e0e599f4bbfbf4a1b8094b05ad7/partiql-tests-data/eval/query/limitoffset.ion#L250 only outputs the following:
org.partiql.errors.TypeCheckException
We should consider adding some more description for certain error messages.
There was a problem hiding this comment.
True! While it's not part of this PR, I'll give some insight.
There's actually a reason for the lack of a stack trace -- performance. From my performance evaluation, one thing was clear: fillInStackTrace is insanely expensive on the CPU. The TypeCheckException removes the default implementation of fillInStackTrace. If you'd like more information, you can temporarily comment it out to figure out where it's coming from. Beyond that, you're totally right. More of the TypeCheckExceptions should have a corresponding error message -- though not all instances have them.
| val comparison = first.compareTo(second) | ||
| return when { | ||
| comparison < 0 -> if (positiveDeltaGood) ICON_CHECK else ICON_CIRCLE_RED | ||
| comparison == 0 -> ICON_CHECK |
There was a problem hiding this comment.
nit: wonder if for if there's no diff, we want some neutral indication consider ➖ (:heavy_minus_sign:). ✅ would seem to indicate some good change but perhaps no regression is good?
There was a problem hiding this comment.
Yeah, I played with doing a grey icon, but it started to look a bit cluttered visually. I figured green/red are pretty easy indicators for reviewers that something is good/bad.
Description
Example of how the new green/red emojis are used to denote good/bad instead of using them as labels. It should make it easier for reviewers to read. Here's how it used to be:
And, here's how it is now:
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.