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.
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.
"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.
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.
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.
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.
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.
This sound ingenious until the test which asserts the wrong behavior somehow slips into production and now that is the new generally desired behavior. May be better tooling with a new test marker is the answer.
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.
pgeorgi | 18 hours ago
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.
[OP] JordiGH | 17 hours ago
I suppose this puts the burden on the human reviewer to ensure that the second commit modifies the prevoiusly passing test.
pgeorgi | 16 hours ago
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.
jbaber | 7 hours ago
That would be significantly easier for me to understand when reading the history of the fix.
epage | 15 hours ago
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.
epage | 17 hours ago
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
FeepingCreature | 16 hours ago
this is why unittest libraries have "known failing" annotations.
nedbat | 14 hours ago
"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.xfaildecorator.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.
mxey | 12 hours ago
Can “expected to fail” catch if your test fails for a different reason than intended?
I don’t know if that’s very useful but also those tests should have a TODO comment either way.
masklinn | 3 hours ago
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.
Pytest supports a
reasonparameter though it is not required by default. Also from experience seeing an XPASS puts you you a different frame of mind than a FAIL.[OP] JordiGH | 3 hours ago
In our case, it's mostly because we're using Java. I added a section addressing this concern:
https://jordi.inversethought.com/blog/the-kluever-maneuver/#:~:text=Shouldn%E2%80%99t%20we%20use%20XFAIL%20instead%3F
jana | 17 hours ago
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.
jackdk | 7 hours ago
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
XFAILand then fix the bug in the commit that removes theXFAILmark.majaha | 17 hours ago
I'm familiar with this from contributing to LLVM.
quad | 4 hours ago
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.
JulianWgs | an hour ago
This sound ingenious until the test which asserts the wrong behavior somehow slips into production and now that is the new generally desired behavior. May be better tooling with a new test marker is the answer.
mxey | 16 minutes ago
How does a test “slip into production”?
ryan-duve | 18 hours ago
Is there a difference between this and TDD?
jdpage | 18 hours ago
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.
icefox | 17 hours ago
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.
mxey | 16 hours ago
I adopted this from the CUE project and they got it from Google.
[OP] JordiGH | 15 hours ago
Hey, I got it from Google too!
ibookstein | 11 hours ago
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.