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

Re: [DISCUSS] Vetoing commits


While I haven't run or even carefully examined the code in question, your
"three line diff" looks compelling given the test results before and after.
Unless I'm missing something, this was not something you proposed on the
JIRA issue, correct? If this were applied on top of the existing patch,
would this resolve your concerns with the patch?

I understand this doesn't fully resolve the issue of vetos in general, but
hopefully we can also move this issue forward.

Michael Mior

Le ven. 10 août 2018 à 04:23, Vladimir Sitnikov <sitnikov.vladimir@xxxxxxxxx>
a écrit :

> 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.
> #1.
> 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,
> isNotNull(isNull(node)));
> I am sure "rexBuilder.makeCall(SqlStdOperatorTable.IS_FALSE"  does count as
> redundant, and `isFalse(...)` should have been used.
> #2.
> 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
> well:
> [ERROR] testIsAlwaysTrueAndFalse(org.apache.calcite.test.RexProgramTest)
> Time elapsed: 0.006 s  <<< FAILURE!
> java.lang.AssertionError:
> Expected: is <true>
>      but: was <false>
> at org.apache.calcite.test.RexProgramTest.checkIs(RexProgramTest.java:2245)
> at
> org.apache.calcite.test.RexProgramTest.testIsAlwaysTrueAndFalse(RexProgramTest.java:2210)
> 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))))
> isAlwaysTrue
> 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"?
> #3.
> Josh>seems to  (originally) be valid, having test cases that were thorough
> enough
> 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
> false".
> 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
> is.
> 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?
> [3]:
> https://github.com/apache/calcite/compare/1c5e7d8b271f2df2a622d4f078118b0f02ea4a1d...d1d6053dffc390952787e64f3fe431d792d10334#diff-e07a78c4cbad5a667f6a712ac303f112R2173
> [4]:
> https://github.com/apache/calcite/pull/787/commits/e26dc09c81334a24c64a5e1bcc8b9270b4725560
> [5]: https://travis-ci.org/apache/calcite/jobs/414265871#L7568-L7569
> Vladimir