Skip to content

Commit

Permalink
Revamp failure testing with query assertions
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
findepi committed Jan 31, 2024
1 parent fe0d836 commit 947fc87
Show file tree
Hide file tree
Showing 85 changed files with 1,274 additions and 986 deletions.
384 changes: 286 additions & 98 deletions core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import org.junit.jupiter.api.parallel.Execution;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;
import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT;

Expand All @@ -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"))
.failure().hasMessageContaining("must be an aggregate expression or appear in GROUP BY clause");

assertThatThrownBy(() -> assertions.query("SELECT v > ANY (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 > ANY (VALUES 1) FROM (VALUES (1, 1), (1, 2)) t(k, v) GROUP BY k"))
.failure().hasMessageContaining("must be an aggregate expression or appear in GROUP BY clause");

assertThatThrownBy(() -> assertions.query("SELECT v > SOME (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 > SOME (VALUES 1) FROM (VALUES (1, 1), (1, 2)) t(k, v) GROUP BY k"))
.failure().hasMessageContaining("must be an aggregate expression or appear in GROUP BY clause");

assertThat(assertions.query("SELECT count_if(v > ALL (VALUES 0, 1)) FROM (VALUES (1, 1), (1, 2)) t(k, v) GROUP BY k"))
.matches("VALUES BIGINT '1'");

assertThat(assertions.query("SELECT count_if(v > ANY (VALUES 0, 1)) FROM (VALUES (1, 1), (1, 2)) t(k, v) GROUP BY k"))
.matches("VALUES BIGINT '2'");

assertThatThrownBy(() -> assertions.query("SELECT 1 > ALL (VALUES k) FROM (VALUES (1, 1), (1, 2)) t(k, v) GROUP BY k"))
.hasMessageContaining("line 1:17: Given correlated subquery is not supported");
assertThat(assertions.query("SELECT 1 > ALL (VALUES k) FROM (VALUES (1, 1), (1, 2)) t(k, v) GROUP BY k"))
.failure().hasMessageContaining("line 1:17: Given correlated subquery is not supported");
}
}
209 changes: 104 additions & 105 deletions core/trino-main/src/test/java/io/trino/sql/query/TestCheckConstraint.java

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import static io.trino.testing.TestingHandles.TEST_CATALOG_NAME;
import static io.trino.testing.TestingSession.testSessionBuilder;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;

@TestInstance(PER_CLASS)
Expand Down Expand Up @@ -533,8 +532,8 @@ public void testRecursion()
.expression("(SELECT orderkey FROM orders)")
.build());

assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessageMatching(".*\\QColumn mask for 'local.tiny.orders.orderkey' is recursive\\E.*");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessageMatching(".*\\QColumn mask for 'local.tiny.orders.orderkey' is recursive\\E.*");

// different reference style to same table
accessControl.reset();
Expand All @@ -549,8 +548,8 @@ public void testRecursion()
.expression("(SELECT orderkey FROM local.tiny.orders)")
.build());

assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessageMatching(".*\\QColumn mask for 'local.tiny.orders.orderkey' is recursive\\E.*");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessageMatching(".*\\QColumn mask for 'local.tiny.orders.orderkey' is recursive\\E.*");

// mutual recursion
accessControl.reset();
Expand All @@ -576,8 +575,8 @@ public void testRecursion()
.expression("(SELECT orderkey FROM orders)")
.build());

assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessageMatching(".*\\QColumn mask for 'local.tiny.orders.orderkey' is recursive\\E.*");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessageMatching(".*\\QColumn mask for 'local.tiny.orders.orderkey' is recursive\\E.*");
}

@Test
Expand All @@ -594,9 +593,9 @@ public void testLimitedScope()
.schema("tiny")
.expression("orderkey")
.build());
assertThatThrownBy(() -> assertions.query(
assertThat(assertions.query(
"SELECT (SELECT min(custkey) FROM customer WHERE customer.custkey = orders.custkey) FROM orders"))
.hasMessage("line 1:34: Invalid column mask for 'local.tiny.customer.custkey': Column 'orderkey' cannot be resolved");
.failure().hasMessage("line 1:34: Invalid column mask for 'local.tiny.customer.custkey': Column 'orderkey' cannot be resolved");
}

@Test
Expand Down Expand Up @@ -635,8 +634,8 @@ public void testInvalidMasks()
.expression("$$$")
.build());

assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessage("line 1:22: Invalid column mask for 'local.tiny.orders.orderkey': mismatched input '$'. Expecting: <expression>");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessage("line 1:22: Invalid column mask for 'local.tiny.orders.orderkey': mismatched input '$'. Expecting: <expression>");

// unknown column
accessControl.reset();
Expand All @@ -651,8 +650,8 @@ public void testInvalidMasks()
.expression("unknown_column")
.build());

assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessage("line 1:22: Invalid column mask for 'local.tiny.orders.orderkey': Column 'unknown_column' cannot be resolved");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessage("line 1:22: Invalid column mask for 'local.tiny.orders.orderkey': Column 'unknown_column' cannot be resolved");

// invalid type
accessControl.reset();
Expand All @@ -667,8 +666,8 @@ public void testInvalidMasks()
.expression("'foo'")
.build());

assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessage("line 1:22: Expected column mask for 'local.tiny.orders.orderkey' to be of type bigint, but was varchar(3)");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessage("line 1:22: Expected column mask for 'local.tiny.orders.orderkey' to be of type bigint, but was varchar(3)");

// aggregation
accessControl.reset();
Expand All @@ -683,8 +682,8 @@ public void testInvalidMasks()
.expression("count(*) > 0")
.build());

assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessage("line 1:10: Column mask for 'orders.orderkey' cannot contain aggregations, window functions or grouping operations: [count(*)]");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessage("line 1:10: Column mask for 'orders.orderkey' cannot contain aggregations, window functions or grouping operations: [count(*)]");

// window function
accessControl.reset();
Expand All @@ -699,8 +698,8 @@ public void testInvalidMasks()
.expression("row_number() OVER () > 0")
.build());

assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessage("line 1:22: Column mask for 'orders.orderkey' cannot contain aggregations, window functions or grouping operations: [row_number() OVER ()]");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessage("line 1:22: Column mask for 'orders.orderkey' cannot contain aggregations, window functions or grouping operations: [row_number() OVER ()]");

// grouping function
accessControl.reset();
Expand All @@ -715,8 +714,8 @@ public void testInvalidMasks()
.expression("grouping(orderkey) = 0")
.build());

assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessage("line 1:20: Column mask for 'orders.orderkey' cannot contain aggregations, window functions or grouping operations: [GROUPING (orderkey)]");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessage("line 1:20: Column mask for 'orders.orderkey' cannot contain aggregations, window functions or grouping operations: [GROUPING (orderkey)]");
}

@Test
Expand Down Expand Up @@ -785,8 +784,8 @@ public void testColumnMaskingUsingRestrictedColumn()
.identity(USER)
.expression("custkey")
.build());
assertThatThrownBy(() -> assertions.query("SELECT orderkey FROM orders"))
.hasMessage("Access Denied: Cannot select from columns [orderkey, custkey] in table or view local.tiny.orders");
assertThat(assertions.query("SELECT orderkey FROM orders"))
.failure().hasMessage("Access Denied: Cannot select from columns [orderkey, custkey] in table or view local.tiny.orders");
}

@Test
Expand All @@ -801,8 +800,8 @@ public void testInsertWithColumnMasking()
.identity(USER)
.expression("clerk")
.build());
assertThatThrownBy(() -> assertions.query("INSERT INTO orders SELECT * FROM orders"))
.hasMessage("Insert into table with column masks is not supported");
assertThat(assertions.query("INSERT INTO orders SELECT * FROM orders"))
.failure().hasMessage("Insert into table with column masks is not supported");
}

@Test
Expand All @@ -817,8 +816,8 @@ public void testDeleteWithColumnMasking()
.identity(USER)
.expression("clerk")
.build());
assertThatThrownBy(() -> assertions.query("DELETE FROM orders"))
.hasMessage("line 1:1: Delete from table with column mask");
assertThat(assertions.query("DELETE FROM orders"))
.failure().hasMessage("line 1:1: Delete from table with column mask");
}

@Test
Expand All @@ -833,12 +832,12 @@ public void testUpdateWithColumnMasking()
.identity(USER)
.expression("clerk")
.build());
assertThatThrownBy(() -> assertions.query("UPDATE orders SET clerk = 'X'"))
.hasMessage("line 1:1: Updating a table with column masks is not supported");
assertThatThrownBy(() -> assertions.query("UPDATE orders SET orderkey = -orderkey"))
.hasMessage("line 1:1: Updating a table with column masks is not supported");
assertThatThrownBy(() -> assertions.query("UPDATE orders SET clerk = 'X', orderkey = -orderkey"))
.hasMessage("line 1:1: Updating a table with column masks is not supported");
assertThat(assertions.query("UPDATE orders SET clerk = 'X'"))
.failure().hasMessage("line 1:1: Updating a table with column masks is not supported");
assertThat(assertions.query("UPDATE orders SET orderkey = -orderkey"))
.failure().hasMessage("line 1:1: Updating a table with column masks is not supported");
assertThat(assertions.query("UPDATE orders SET clerk = 'X', orderkey = -orderkey"))
.failure().hasMessage("line 1:1: Updating a table with column masks is not supported");
}

@Test
Expand Down Expand Up @@ -906,12 +905,12 @@ public void testColumnMaskWithHiddenColumns()
.assertThat()
.skippingTypesCheck()
.matches("VALUES 'POLAND'");
assertThatThrownBy(() -> assertions.query("INSERT INTO mock.tiny.nation_with_hidden_column SELECT * FROM mock.tiny.nation_with_hidden_column"))
.hasMessage("Insert into table with column masks is not supported");
assertThatThrownBy(() -> assertions.query("DELETE FROM mock.tiny.nation_with_hidden_column"))
.hasMessage("line 1:1: Delete from table with column mask");
assertThatThrownBy(() -> assertions.query("UPDATE mock.tiny.nation_with_hidden_column SET name = 'X'"))
.hasMessage("line 1:1: Updating a table with column masks is not supported");
assertThat(assertions.query("INSERT INTO mock.tiny.nation_with_hidden_column SELECT * FROM mock.tiny.nation_with_hidden_column"))
.failure().hasMessage("Insert into table with column masks is not supported");
assertThat(assertions.query("DELETE FROM mock.tiny.nation_with_hidden_column"))
.failure().hasMessage("line 1:1: Delete from table with column mask");
assertThat(assertions.query("UPDATE mock.tiny.nation_with_hidden_column SET name = 'X'"))
.failure().hasMessage("line 1:1: Updating a table with column masks is not supported");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import org.junit.jupiter.api.parallel.Execution;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;
import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT;

Expand Down Expand Up @@ -85,13 +84,13 @@ public void testSelectAllAliases()
@Test
public void testColumnAliasing()
{
assertThatThrownBy(() -> assertions.query("SELECT DISTINCT 1 AS a, a + b FROM (VALUES (1, 2)) t(a, b) ORDER BY a + b"))
.hasMessage("line 1:1: For SELECT DISTINCT, ORDER BY expressions must appear in select list");
assertThat(assertions.query("SELECT DISTINCT 1 AS a, a + b FROM (VALUES (1, 2)) t(a, b) ORDER BY a + b"))
.failure().hasMessage("line 1:1: For SELECT DISTINCT, ORDER BY expressions must appear in select list");

assertThatThrownBy(() -> assertions.query("SELECT DISTINCT -a AS a, a + b FROM (VALUES (1, 2)) t(a, b) ORDER BY a + b"))
.hasMessage("line 1:1: For SELECT DISTINCT, ORDER BY expressions must appear in select list");
assertThat(assertions.query("SELECT DISTINCT -a AS a, a + b FROM (VALUES (1, 2)) t(a, b) ORDER BY a + b"))
.failure().hasMessage("line 1:1: For SELECT DISTINCT, ORDER BY expressions must appear in select list");

assertThatThrownBy(() -> assertions.query("SELECT DISTINCT a, a + b FROM (VALUES (1, 2)) t(a, b) ORDER BY a + b"))
.hasMessage("line 1:1: For SELECT DISTINCT, ORDER BY expressions must appear in select list");
assertThat(assertions.query("SELECT DISTINCT a, a + b FROM (VALUES (1, 2)) t(a, b) ORDER BY a + b"))
.failure().hasMessage("line 1:1: For SELECT DISTINCT, ORDER BY expressions must appear in select list");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import org.junit.jupiter.api.parallel.Execution;

import static io.trino.spi.StandardErrorCode.SYNTAX_ERROR;
import static io.trino.testing.assertions.TrinoExceptionAssert.assertTrinoExceptionThrownBy;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;
import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT;
Expand Down Expand Up @@ -60,20 +59,24 @@ public void testQuotesInStatement()
@Test
public void testSyntaxError()
{
assertTrinoExceptionThrownBy(() -> assertions.query("EXECUTE IMMEDIATE 'SELECT ''foo'"))
assertThat(assertions.query("EXECUTE IMMEDIATE 'SELECT ''foo'"))
.failure()
.hasErrorCode(SYNTAX_ERROR)
.hasMessageMatching("line 1:27: mismatched input '''. Expecting: .*");
assertTrinoExceptionThrownBy(() -> assertions.query("EXECUTE IMMEDIATE\n'SELECT ''foo'"))
assertThat(assertions.query("EXECUTE IMMEDIATE\n'SELECT ''foo'"))
.failure()
.hasErrorCode(SYNTAX_ERROR)
.hasMessageMatching("line 2:8: mismatched input '''. Expecting: .*");
}

@Test
public void testSemanticError()
{
assertTrinoExceptionThrownBy(() -> assertions.query("EXECUTE IMMEDIATE 'SELECT * FROM tiny.tpch.orders'"))
assertThat(assertions.query("EXECUTE IMMEDIATE 'SELECT * FROM tiny.tpch.orders'"))
.failure()
.hasMessageMatching("line 1:34: Catalog 'tiny' not found");
assertTrinoExceptionThrownBy(() -> assertions.query("EXECUTE IMMEDIATE\n'SELECT *\nFROM tiny.tpch.orders'"))
assertThat(assertions.query("EXECUTE IMMEDIATE\n'SELECT *\nFROM tiny.tpch.orders'"))
.failure()
.hasMessageMatching("line 3:6: Catalog 'tiny' not found");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import org.junit.jupiter.api.parallel.Execution;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;
import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT;

Expand Down Expand Up @@ -65,8 +64,8 @@ public void testInShortCircuit()
assertThat(assertions.query("SELECT IF(3 IN (2, 4, 3, 5 / 0), 1e0, x + x) FROM (VALUES rand()) t(x)")).matches("VALUES 1e0");

// the in-predicate is inlined into Values and evaluated by the ExpressionInterpreter: eager evaluation, failure.
assertThatThrownBy(() -> assertions.query("SELECT 3 IN (2, 4, 3, 5 / 0)"))
.hasMessage("Division by zero");
assertThat(assertions.query("SELECT 3 IN (2, 4, 3, 5 / 0)"))
.failure().hasMessage("Division by zero");
}

@Test
Expand Down
Loading

0 comments on commit 947fc87

Please sign in to comment.