Developers debate tradeoffs between test approaches
4 Oct 31 1:16 PM · 11d ago · 8 comments · 1 source · development 4 of 4
Discussion emerges over whether 'expected to fail' annotations or explicit tests of buggy behavior better serve code review clarity and maintainability, with no consensus reached.
“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.”
epageJordi Gómez Hernández Developer and writerKurt Kluever Developer (originator of technique)
The whole story postscomments the bright band is this development · numbered dots are the others · click one to jump
What people said 24 voices · best of 30 · verbatim
-
"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…
-
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.
-
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.
-
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
-
this is why unittest libraries have "known failing" annotations.
-
> 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…
-
Bisecting, commit atomicity, general code hygiene, just to name a few?
-
If you're bisecting a (new) failing test, having test failures in your history makes that harder to automate.
-
Unless you squash-merge (in which case I would write the breaking test locally, fix it, and commit a single change with just the new passing test), your proposal still means that green-but-broken tests will make it onto `master` and then you could get dubious `git bisect` results, etc.
-
In our case, it's mostly because we're using Java. I added a section addressing this concern:
-
That would be significantly easier for me to understand when reading the history of the fix.
-
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…
-
Can you not bisect when there's a test failing? Commit atomicity as far as I understand is a concept that (often) includes the requirement that tests pass after every commit, but to me it's still not clear why having a failing CI on some commit is a bad thing. General code hygeine is very general and subjective and again, I don't see the problem…
-
this seems good if you're already heavy on process in a large team. i would want to require the bug/fixme comment as part of the process, when doing this. you could even commit the reproduction before the fix, as part of triage, if it's annotated as `[BugReproduction(issue#)]` or something so that if someone breaks the test they can tell it wasn't…
-
Probably multiple ways. The first one I can think of: The commit introducing the fix introduces another bug and gets reverted by another developer who is not familiar with the Kluever Maneuver. Development can be messy and slip ups happen. I don‘t think this approach is bad, but I think it breaks the assumption that a commit change is atomic.
-
It no doubt sounds like a negative act first when you frame it like that. I think a _worse_ framing of the situation is that broken behavior is already committed and is also not covered by test. Viewed like that, you are first raising the code coverage and then demonstrably correcting a bug, both now positive acts on the codebase.
-
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.
-
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.
-
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.
-
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.
-
My PRs get squashed into main so I dunno. Maybe it helps the programmer figure out what was wrong, but it doesn't persist in git history
-
I think an `XFAIL` test achieves your objective better. It raises the code coverage and documents the misbehaviour.
-
That's a great collection. I really like most of the suggestions. My only divergence would be on keeping a clean gut biaect. I expect my PRs to be squashed and I don't expect anyone to bisect my branch. As a reviewer it's trivially easy to see one small pattern of change repeated N times and know it's the same pattern, but one commit with N…
-
Bisecting branches can be important. As an example, I've ported a template engine from one parser to another, taking careful small steps to break things up and be bisect friendly and they squashed it. It would have been easy to introduce a bug by accident. Even breaking the port down to 5 or so PRs, they were still large and bisecting to one of…
All 4 developments of The Kluever Maneuver: Writing Tests That Fail Before Fixing… →
LobstersHacker News