-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[draft][v1] Re-enable some randomized tests #1696
base: main
Are you sure you want to change the base?
Conversation
CROSS-ENGINE-REPORT ❌
Testing Details
Result Details
Now FAILING Tests ❌The following 2 test(s) were previously PASSING in BASE but are now FAILING in TARGET: Click here to see
Now IGNORED Tests ❌The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. Now Passing Tests180 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-FC89DB0). Before merging, confirm they are intended to pass. The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. CROSS-COMMIT-REPORT ✅
Testing DetailsResult Details
|
@@ -1,121 +0,0 @@ | |||
// package org.partiql.lang.randomized.eval.builtins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(self-review) TimestampTemporalAccessor
class was deleted in v1 branch so tests no longer relevant
@@ -53,7 +52,6 @@ class PartiQLParserDateTimeRandomizedTests { | |||
} | |||
|
|||
@Test | |||
@Disabled("The planner does not return the v1 plans right now. See assertExpression.") // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(self-review) these tests passed when re-enabled.
There is another file that has some datetime evaluation tests -- https://github.com/partiql/partiql-lang-kotlin/blob/enable-some-randomized-tests/test/partiql-randomized-tests/src/test/kotlin/org/partiql/lang/randomized/eval/EvaluatingCompilerDateTimeRandomizedTests.kt. Those tests depend on the old datetime modeling. Once that modeling is updated in #1691, we can try re-enabling those tests.
* TODO int overflow behavior is confusing. Should we | ||
* - Overflow (no error) | ||
* - Upcast to long (no error) | ||
* - Error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(self-review) cut an issue to track -- #1697.
Will discuss w/ team on whether the evaluation behavior needs changed. If so, how to update the randomized tests.
Relevant Issues
Description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.