[v1] Fix try-catch for conformance tests expecting an error #1542
Conversation
Conformance comparison report-Cross Engine
Number failing in both: 77 Number passing in legacy engine but fail in eval engine: 157 Number failing in legacy engine but pass in eval engine: 468 Conformance comparison report-Cross Commit-LEGACY
Number failing in both: 441 Number passing in Base (e21a1eb) but now fail: 104 Number failing in Base (e21a1eb) but now pass: 2 Click here to see
Conformance comparison report-Cross Commit-EVAL
Number failing in both: 161 Number passing in Base (e21a1eb) but now fail: 74 Number failing in Base (e21a1eb) but now pass: 1 Click here to see
|
alancai98
changed the title
|
Some additional test failures due to this change -- https://github.com/partiql/partiql-lang-kotlin/actions/runs/10292632996. Some of these errors are properly caught by the planner but not detected by the evaluator. Regarding the planner errors, we should probably throw/error on these cases -- #1543. |
There was a problem hiding this comment.
This PR makes me think, why are we even catching these exceptions? Using exceptions for business logic is generally advised against. IMO, this whole method can be simplified by creating the statement/actual variables in each assertion block. Then, we can use a try-catch just for the Assertion.EvaluationFailure where it's required.
Though, it would also be nice to have each variant of TestCase implement their own assertion logic.
| @@ -36,14 +38,16 @@ class TestRunner<T, V>(private val factory: TestExecutor.Factory<T, V>) { | |||
| val expect = executor.fromIon(assertion.expectedResult) | |||
| if (!executor.compare(actual, expect)) { | |||
| val ion = executor.toIon(actual) | |||
| error("Expected: ${assertion.expectedResult}\nActual: $ion\nMode: ${case.compileOptions.typingMode}") | |||
| throw TestRunnerException("Expected error to be thrown but none was thrown.\n${case.name}\nActual result: $ion") | |||
There was a problem hiding this comment.
This error message isn't the intention behind Assertion.EvaluationSuccess.
Yeah the current |
Relevant Issues
Description
EvaluationFailbut did not errorOther Information
Updated Unreleased Section in CHANGELOG: [NO]
Any backward-incompatible changes? [NO]
Any new external dependencies? [NO]
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES]
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.