Enable tests to fail #22693
Enable tests to fail #22693
Conversation
elharo
force-pushed
the
fail2
branch
2 times, most recently
from
d42991c
to
169c551
Compare
| * There are so many ways for matches to fail that this is not likely to be generally useful. | ||
| * Pending better diagnostics, please leave this here, and restrict its use to simple queries | ||
| * that have few ways to not match a pattern, and functionality that is well-tested with | ||
| * positive tests. | ||
| */ | ||
| private void assertFails(Runnable runnable) | ||
| private void assertPlanFails(Runnable runnable) |
There was a problem hiding this comment.
I was going to comment on the function name, since this is looking for plan mismatches and not planning failures, but actually I feel this whole function isn't necessary. PlanAssert already has assertPlanDoesNotMatch, which checks the match result directly and prints a nice error message. I think the tests using this should call to that directly instead of calling a function that calls assertPlan() and then inverting it.
There was a problem hiding this comment.
Makes sense. It would be nicer to reuse more code and remove this completely. That this exists at all is probably just a relic of code written over multiple years by different devs who didn't always know exactly what already existed where. (I certainly don't.) The existing code is a little convoluted so it might take a minute to rejigger the tests to use PlanAssert.
| output(ImmutableList.of("ORDERKEY", "EXTENDEDPRICE"), | ||
| strictTableScan("lineitem", ImmutableMap.of("ORDERKEY", "orderkey"))))); | ||
| String sql = "SELECT orderkey, extendedprice FROM lineitem"; | ||
| ImmutableList<String> outputs = ImmutableList.of("ORDERKEY", "EXTENDEDPRICE"); |
There was a problem hiding this comment.
| ImmutableList<String> outputs = ImmutableList.of("ORDERKEY", "EXTENDEDPRICE"); | |
| List<String> outputs = ImmutableList.of("ORDERKEY", "EXTENDEDPRICE"); |
| ImmutableList<String> outputs = ImmutableList.of("ORDERKEY", "EXPRESSION"); | ||
| ImmutableMap<String, ExpressionMatcher> assignments = ImmutableMap.of("EXPRESSION", expression("1 + ORDERKEY"), "ORDERKEY", expression("ORDERKEY")); |
There was a problem hiding this comment.
| ImmutableList<String> outputs = ImmutableList.of("ORDERKEY", "EXPRESSION"); | |
| ImmutableMap<String, ExpressionMatcher> assignments = ImmutableMap.of("EXPRESSION", expression("1 + ORDERKEY"), "ORDERKEY", expression("ORDERKEY")); | |
| List<String> outputs = ImmutableList.of("ORDERKEY", "EXPRESSION"); | |
| Map<String, ExpressionMatcher> assignments = ImmutableMap.of("EXPRESSION", expression("1 + ORDERKEY"), "ORDERKEY", expression("ORDERKEY")); |
Description
This is a bug pattern I haven't seen before. The AssertionError thrown by the fail method was caught by an overly broad catch clause so the test couldn't fail. This wouldn't have happened with test first development.
Motivation and Context
Tests should fail when broken
Impact
none
Test Plan
CI
Contributor checklist
Release Notes