-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Revamp failure testing with query assertions #20509
Conversation
aab4474
to
888f4d8
Compare
cc @trinodb/maintainers |
|
888f4d8
to
b57cc19
Compare
@@ -38,22 +37,22 @@ public void teardown() | |||
@Test | |||
public void testQuantifiedComparison() | |||
{ | |||
assertThatThrownBy(() -> assertions.query("SELECT v > ALL (VALUES 1) FROM (VALUES (1, 1), (1, 2)) t(k, v) GROUP BY k")) | |||
.hasMessageContaining("must be an aggregate expression or appear in GROUP BY clause"); | |||
assertThat(assertions.query("SELECT v > ALL (VALUES 1) FROM (VALUES (1, 1), (1, 2)) t(k, v) GROUP BY k")) |
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 like the new API :)
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.
thank you!
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.
@findepi Can we extract changes to the QueryAssertions
in the last commit to the separate commit as you are mixing mechanical (regex) changes and manual ones in a single commit?
There are related failures |
The changes are tightly related, I don't see a way to split them. |
`ensureOrdering` was always false, so effectively unused.
b57cc19
to
c57ac24
Compare
Previously, the `QueryAssert.query` could represent original SQL query, or a description (in case of column projections). This commit separates those duties. Original query is not available in case of projections because it would not produce the current (projected) result.
b09982c
to
86afd6b
Compare
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 % failures
The new API requires that failures are a TrinoException (and has an scape hatch for cases where they are not). No plan to fix these in this PR, but they will now be easy to spot (any usages of |
Update places that depended on `query(...)` query assert constructor executing the query. These should use `execute`, `assertUpdate`, etc.
86afd6b
to
457cef5
Compare
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.
looks nice
BigQuery related:
|
Before the introduction of query assertions (the `QueryAssert` class), `assertQueryFails` was used to verify that a query fails with expected error message AND the failure exception type is `TrinoException`. With query assertions, the typical pattern for testing query failures became: assertThatThrownBy(() -> query(...)) .... This unfortunately does not verify that the exception type is the `TrinoException`. The existing alternative: assertTrinoExceptionThrownBy(() -> query(...)) ... is rarely used, perhaps because it's longer and the established shorter pattern is followed. Both above methods share the disadvantage of basing on the `QueryAssert` construction side-effect of evaluating the query eagerly. This commit revamps `QueryAssert` to make failures first class citizen from `QueryAssert` perspective, on par with query results. Expected failures are verified to be `TrinoException` type. The result is more fluent API, as both success and failures assertions are available on the same query assert object, and thus can be auto-completed by an IDE. From code updating perspective, most of the code updates fall into two categories: * failure testing. This functionality is available from query assert via `.failure()` accessor. Code was updated with regular expression - search for: `(assertThatThrownBy|assertTrinoExceptionThrownBy)\(\(\) -> (assertions\.)?query\(` - replace with: `assertThat($2query(` - add `.failure()` to access assertions on the failure exception (use `nonTrinoExceptionFailure()` for cases where query incorrectly fails without a proper `TrinoException`) * detailed inspection of `MaterializedResult` objects. This lower-level functionality is available from query assert via `.result()` accessor.
457cef5
to
947fc87
Compare
Before the introduction of query assertions (the
QueryAssert
class),assertQueryFails
was used to verify that a query fails with expectederror message AND the failure exception type is
TrinoException
.With query assertions, the typical pattern for testing query failures
became:
This unfortunately does not verify that the exception type is the
TrinoException
. The existing alternative:is rarely used, perhaps because it's longer and the
established shorter pattern is followed.
Both above methods share the disadvantage of basing on the
QueryAssert
construction side-effect of evaluating the query eagerly.
This commit revamps
QueryAssert
to make failures first class citizenfrom
QueryAssert
perspective, on par with query results. Expectedfailures are verified to be
TrinoException
type. The result is morefluent API, as both success and failures assertions are available on the
same query assert object, and thus can be auto-completed by an IDE.
From code updating perspective, most of the code updates fall into two
categories:
failure testing. This functionality is available from query assert via
.failure()
accessor. Code was updated with regular expression(assertThatThrownBy|assertTrinoExceptionThrownBy)\(\(\) -> (assertions\.)?query\(
assertThat($2query(
.failure()
to access assertions on the failure exception.(use
nonTrinoExceptionFailure()
for cases where query incorrectlyfails without a proper
TrinoException
)detailed inspection of
MaterializedResult
objects. This lower-levelfunctionality is available from query assert via
.result()
accessor.