-
Notifications
You must be signed in to change notification settings - Fork 25.7k
ESQL: Time_zone tests #136748
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
ESQL: Time_zone tests #136748
Conversation
… ones that use it
# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateFormatTests.java
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.
This depends on the locale, so a random locale would break the tests
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.
This depends on the locale, so a random locale would break the tests
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.
We're testing the configuration "now()", so we reuse it. It could be randomized too
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.
This depends on both the zoneId and the locale
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.
This depends on both the zoneId and the locale
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.
This depends on the locale.
As unary() and sibling functions doesn't accept a custom logic on testcases, I added that "mapTestCase()" at the end.
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.
This depends on the zoneId
| assertNotEquals(expr, differentExpr); | ||
| } | ||
|
|
||
| private static Configuration randomConfiguration() { |
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.
Reusing ConfigurationTestUtils
...in/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/TestCaseSupplier.java
Show resolved
Hide resolved
| if (line.endsWith("\\;")) { | ||
| // SET statement with escaped ";" |
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.
We could use other mechanism; I just chose a typical one here. Listening for opinions
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.
I guess we have plenty of queries that end with a ;, so the practical alternatives to this would be
- change hundreds of tests and move the
;to a new line - give up and always have a SET followed by a FROM on the same line.
In conclusion, your solution seems reasonable
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| public Object parse(String line) { | ||
| // read the query | ||
| if (testCase == null) { | ||
| if (line.startsWith(SCHEMA_PREFIX)) { |
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.
This came from QL; I removed it here. It wasn't being used anywhere
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.
👍
luigidellaquila
left a comment
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.
LGTM, thanks @ivancea!
You only have one failing test due to randomization, but the problem is not in your PR (the test needs a fix)
...in/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/TestCaseSupplier.java
Show resolved
Hide resolved
| public Object parse(String line) { | ||
| // read the query | ||
| if (testCase == null) { | ||
| if (line.startsWith(SCHEMA_PREFIX)) { |
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.
👍
| if (line.endsWith("\\;")) { | ||
| // SET statement with escaped ";" |
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.
I guess we have plenty of queries that end with a ;, so the practical alternatives to this would be
- change hundreds of tests and move the
;to a new line - give up and always have a SET followed by a FROM on the same line.
In conclusion, your solution seems reasonable
# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java
Continuation of elastic#136637 Configure tests to work with SET, and add/update tests for: - Request "time_zone" parameter - "SET" "time_zone" parameter - Functions currently affected by `Configuration.zoneId` Also, use random configurations by default for unit tests, so we catch and update functions not testing configuration changes.
Continuation of #136637
Configure tests to work with SET, and add/update tests for:
Configuration.zoneIdAlso, use random configurations by default for unit tests, so we catch and update functions not testing configuration changes.