Calcite tests remove expected exception #16046
Conversation
* update testcases using `expectedException` to utilize `assertThrows` instead * remove `BaseCalciteQueryTest#expectedException` * fixes `cannotVectorize` so it doesn't anymore stops further processing * `msqIncompatible` is not anymore toggles a boolean - its an `Assume` instead Fixes apache#15423
github-actions
bot
added
Area - Batch Ingestion
Area - Querying
Area - Dependencies
Area - MSQ
|
failure of jdk11 static checks seem unrelated |
…e-expected-exception
There was a problem hiding this comment.
Thanks @kgyrtkirk - nice cleanup overall. I left a few suggestions.
sql/src/test/java/org/apache/druid/sql/calcite/CalciteParameterQueryTest.java
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/CalciteParameterQueryTest.java
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/CalciteParameterQueryTest.java
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
Outdated
Show resolved
Hide resolved
| ImmutableList.of() | ||
| ); | ||
| }); | ||
| assertTrue(exception.getMessage().contains("The number of values in the IN clause for [dim6] in query exceeds configured maxNumericFilter limit of [2] for INs. Cast [3] values of IN clause to String")); |
There was a problem hiding this comment.
nit: can you please break this up to accommodate smaller screen length?
Also, please use one consistently between MatcherAssert.assertThat and assertTrue (given assertThat is deprecated).
There was a problem hiding this comment.
I think its better to use static imports than write out the class the actual assert/assume/etc comes from;
if the usecase would require to use 2 of those static imported methods from 2 different sources at the same time I would see differently - but if there is only one then I think it as something which reduces the readability with no added benefit
| ImmutableList.of() | ||
| ); | ||
| }); | ||
| assertTrue(exception.getMessage().contains("__time column")); |
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java
Outdated
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java
Outdated
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java
Outdated
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java
Outdated
Show resolved
Hide resolved
|
thank you @abhishekrb19 for taking a look; I plan to later also run |
sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java
Outdated
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/CalciteSubqueryTest.java
Outdated
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
Outdated
Show resolved
Hide resolved
| @@ -295,15 +298,11 @@ public static Map<String, Object> getTimeseriesContextWithFloorTime( | |||
| public final SqlEngine engine0; | |||
| final boolean useDefault = NullHandling.replaceWithDefault(); | |||
|
|
|||
| @Rule(order = 1) | |||
| public ExpectedException expectedException = ExpectedException.none(); | |||
|
|
|||
| @Rule(order = 2) | |||
There was a problem hiding this comment.
We can also remove the junit rule order or re-order the rules (same applies to @Rule(order = 3) queryFrameworkRule:
| @Rule(order = 2) | |
| @Rule |
There was a problem hiding this comment.
I agree that these could be changed; however one of the followup changes is to upgrade calcite* tests to junit5 - can we postpone this cleanup to that patch?
|
The rule ordering cleanup changes can come in a later patch |
expectedExceptionto utilizeassertThrowsinsteadBaseCalciteQueryTest#expectedExceptioncannotVectorizeso it doesn't anymore stops further processingmsqIncompatibleis not anymore toggles a boolean - its anAssumeinsteadFixes #15423
This PR has: