The Kluever Maneuver
44 points by JordiGH
44 points by JordiGH
https://aegis.sourceforge.net/ went a step further and expected only the correct test, but had some rules on how it can backport it separate from the fix.
So you submit a change, which comes with a test (for new features, the test would fail because there's no feature to exercise), and aegis would only merge the change if the test-on-its-own failed and passes with the change.
In "write a fix demonstrating the bug (by passing)" you lose the invariant that the very same test failed, then passed. That is, the fix might be unrelated to the test's behavior.
I suppose this puts the burden on the human reviewer to ensure that the second commit modifies the prevoiusly passing test.
https://lobste.rs/c/72mqyb proposes using "expect fail" for such tests at first, which encodes the expectation in a way that is verifiable, and easy to flip (and to verify that change to the test) when the fix arrives.
For me, a key point of this is to make the change of behavior clear, complementing the PR description. If an "expect fail" doesn't clearly communicate what operation failed and how, then I'd rather switch the code to assert that condition.
We independently developed this in Cargo and it is a big help. It cuts a commit down to the esentials to better see what changed and it is a much better illustration of what a PR does than the description.
My write up for this is at https://epage.github.io/dev/pr-style/#c-test
this is why unittest libraries have "known failing" annotations.
"Known failing" would let you do it in a more natural way: write the test for the correct behavior, and mark it as "expected to fail". Then you can commit the fix to the code, with the change to remove the fail expectation.
The Kluever Maneuver seems odd to me: write the test confirming the wrong behavior. I'd rather have the first commit confirm the correct behavior with the @pytest.mark.xfail decorator.
Doing it this way could also let you separate the two commits by a long period of time without having misleading tests in the code.
Can “expected to fail” catch if your test fails for a different reason than intended?
Doing it this way could also let you separate the two commits by a long period of time without having misleading tests in the code.
I don’t know if that’s very useful but also those tests should have a TODO comment either way.
Pytest’s XFAIL can take an exception type, but it’s absolutely possible for a given exception kind to be raised for an unexpected reason, so strictly speaking no.
I don’t know if that’s very useful but also those tests should have a TODO comment either way.
Pytest supports a reason parameter though it is not required by default. Also from experience seeing an XPASS puts you you a different frame of mind than a FAIL.
In our case, it's mostly because we're using Java. I added a section addressing this concern:
I don't think I like committing passing but known-wrong tests to the codebase, even if immediately patched. But I could see the argument if you mark the test XFAIL and then fix the bug in the commit that removes the XFAIL mark.
I love this. It's one of the things I very often ask new contributors to rust-lang/rust to do when reviewing, and it's become kind of a standard. Any bug fix pr is two commits, and imo that's extremely important.
But what if the test accidentally already passed before the alleged bugfix and the alleged bugfix didn’t actually work? In that case the added test gives false confidence that the bugfix worked.
Why do we have confidence that the new test is any better implemented? It could accidentally be passing the implementation in commit 2 too. This intrinsic problem is why the TDD guys focus on the Red-Green-Refactor cycle.
Is there a difference between this and TDD?
A couple, yeah. TDD would have you write a test asserting the correct behavior that initially fails, while this methodology has you write a test asserting the incorrect that initially passes, but must be fixed.
Also, more broadly, TDD is a practice that asserts that any code change, from new features to bug fixes, should be preceded by the creation of a test suite. Whether or not this is useful or not has long been a subject of debate.
Personally I've found writing a (failing) regression test first to be useful, followed by a fix that makes it pass. I don't mind failing test suites in branches, however. If I did mind, I might be more inclined to mark the test as expected to fail, rather than modifying the test itself when I fixed the bug.
I usually do this by adding a test for the fixed bug and a test that demonstrates the bug in action, but this is an interesting modification of the idea.
One related thing I like using is pytest's xfail, with strict=True. You can then write the test to expect the desired behavior rather than any specific incorrect behavior, and you'll be obligated to remove the xfail when you fix the bug.