[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [DISCUSS] Vetoing commits

Josh>3-part veto from [1]. The first cites maintainability/readability of
Josh>unit tests. This feels more subjective than objective, and against the
Josh>spirit of a veto

Josh, are you sure you did check the contents of pull/778 as of 7 Aug 2018
08:00 UTC? (which is the timestamp of [1] comment)

I fully agree that "maintainability/readability" is undecidable problem in
general, yet CALCITE-2438 is special, and it is trivial to decide.


Please check [3] which is the diff of pull/778 as of the timestamp [1].
The original test used 15 lines of code to verify just 2 cases (not 2
flavours of input data, but the total number of inputs is just 2).

Josh>I am pessimistic that any of these test-cases could ever be
Josh>called "concise" (multiple, nested negations)

I had no issues with that since the test was trying to validate if the
system could handle cases like that.
On contrary "absence of cases for multiple nested negations" would count as
"missing tests".

Sample from pull/778>rexBuilder.makeCall(SqlStdOperatorTable.IS_FALSE,

I am sure "rexBuilder.makeCall(SqlStdOperatorTable.IS_FALSE"  does count as
redundant, and `isFalse(...)` should have been used.

Josh>solely based on the preference of  test case failure messages

Let me show you and example.
I've created a commit [4] (which comes on top of [2]). [4] intentionally
breaks the code to check how the test responds.

You can find test execution report in [5], however I would cite it here as

[ERROR] testIsAlwaysTrueAndFalse(org.apache.calcite.test.RexProgramTest)
Time elapsed: 0.006 s  <<< FAILURE!
Expected: is <true>
     but: was <false>
at org.apache.calcite.test.RexProgramTest.checkIs(RexProgramTest.java:2245)

As of now, Apache Calcite has 68 open pull requests, and it is important
that test failures produce meaningful errors rather than obscure "expected
is<true> but: was <false>". It makes it simpler for reviewers to analyze
the failure and suggest changes.
Can you tell which of the cases got broken? Of course you can't.
Remember, that pull request contents changes over time as new commits
added, so and this kind of failure becomes highly tied to the commit sha.
Note that "running mvn test" is the recommended way to check code by Apache
Calcite contribution guide, and it produces the same unreadable message.

That is: it complicates life of a developer, and it complicates life of a
reviewer as well.

To justify my point, I would suggest how a mere 3 line diff could make a
dramatic difference:

   /** Checks that {@link RexNode#isAlwaysTrue()},
    * {@link RexNode#isAlwaysTrue()} and {@link RexSimplify} agree that
    * an expression reduces to true or false. */
   private void checkIs(RexNode e, boolean expected) {
-    assertThat(e.isAlwaysTrue(), is(expected));
-    assertThat(e.isAlwaysFalse(), is(!expected));
-    assertThat(simplify(e).toString(), is(expected ? "true" : "false"));
+    assertEquals(e + " isAlwaysTrue", expected, e.isAlwaysTrue());
+    assertEquals(e + " isAlwaysFalse", !expected, e.isAlwaysFalse());
+    assertEquals(e + " should simplify to " + expected,
+        expected ? "true" : "false", simplify(e).toString());

Are those lines hard to implement? Of course not.
Now the error message becomes

java.lang.AssertionError: IS NOT TRUE(NOT(IS NOT NULL(IS NULL($0))))
Expected :true
Actual   :false

Now the error gives immediate feedback, and it enables developer/reviewer
to compute the expression on a back of a napkin and assess if the bug is in
the code and/or test assertion. The test becomes way easier to maintain if
it fails ever. The failure is way easier to read by a brand new developer.

Do you still consider the veto to be "solely based on the preference"?

Josh>seems to  (originally) be valid, having test cases that were thorough

Well, that is never enough tests, yet I am sure [2] has not enough tests. I
have not flagged it second time to avoid blur of the subject.

I do find the culture of "good assert errors" to be equally important as
the culture of "appropriate commit messages" and/or the culture of
"spelling errors in the code", so I just ignored lack of tests in [2] to
focus on the error messages.

Re technical flaws in tests:

[2] creates NON-nullable inputs only. That is all the "isNull" checks
always short-circuit to false.
Should the test exercise nullable inputs? I am sure it is a valid and
obvious technical reason to claim that [2] lacks tests.

[2] validates only two types of tests "always true", and "always false".
The test does not explore cases that are neither "always true" nor "always

For instance: isNull(not(nullableX)), it would fail (as of [2]) as follows
(note it fails with a hidden assertion in Calcite code, not in the test
code itself):
java.lang.AssertionError: result mismatch: when applied to {$10=true}, IS
NULL(NOT($10)) yielded false, and IS NOT NULL($10) yielded true
at org.apache.calcite.rex.RexSimplify.verify(RexSimplify.java:1138)
at org.apache.calcite.rex.RexSimplify.simplify(RexSimplify.java:175)

For instance: if I alter implementation of `RexCall#isAlwaysTrue` as
follows, then [2] does not catch the bug.
     case NOT:
-      return operands.get(0).isAlwaysFalse();
+      return !operands.get(0).isAlwaysTrue();

Is it a technical justification for the "lack of tests in [2]"? I'm sure it
Should I have provided all the failure cases upfront? I doubt so since I
would basically have to implement the issue on my own.
What is the point of reviewing then if it forces me to implement the full
test code?

[5]: https://travis-ci.org/apache/calcite/jobs/414265871#L7568-L7569