0

Let's say I want to add tests to a software that has some flaws/bugs.

I want to add the tests before I fix the bugs. And I want to actually merge them in the main branch, not just have them in a feature branch or bugfix branch somewhere.
(EDIT: See below as to why I might want to do this.)

If this is relevant, I am working with phpunit. But I am looking for more general answers.

I see the following options:

  1. Create failing tests that describe the desired behavior, and merge those into the main branch. This means any QA pipeline has to be suppressed.
  2. Create passing tests that describe the current (flawed) behavior, and merge those into the main branch. Add @todo comments if possible. Update them later as part of the bug fix.
  3. Use a fancy mechanism that allows switching between "desired" mode and "current" mode. Running the tests in "desired" mode would make them fail, but running them in "current" mode would make them pass.
  4. Don't merge the tests, until I fix them.

Personally, for my own projects, I like options 2 and 3.

But when working in a team, I want to be able to justify my strategies with online references, instead of just personal opinion.

So, is there any named "best practice" or "pattern" that matches option 2 or 3 above? Or am I missing something, and there are even better strategies available?

EDIT: Why merge the tests before fixing the bug?

(I am adding this section to avoid having to put all of it into the comments.)

Why would I want to merge the tests into the main branch, before the bug fix? This has been a contention in the comments, so I am summarizing possible reasons here.

Possible reasons why we can't fix the bug now:

  • The bug fix might be risky or disruptive, perhaps we want to postpone it for a new major version.
  • The bug fix, unlike the test, needs dedicated effort by the maintainer, whereas the new test can be developed by a contributor.
  • There is no budget / resources to fix the bug now.
  • The existing behavior is not fully understood, and we don't really understand the implications that fixing it would have.
  • We know the current behavior is wrong, but we don't know have precise spec for the desired behavior. Perhaps we need feedback from the client or management.
  • The bug has been discovered while doing something else, and fixing it now is out of scope.

So why would we want to merge the tests anyway?

(assuming option 2 from above)

  • The tests cover more than just this one bug.
  • We want to reward and credit contributors who provide tests, even before fixing a bug.
  • We want to provide a starting point for contributors who attempt to fix the bug.
  • We want a git diff of the fix to give a precise overview of behavior change.
  • We want to detect when the bug might be fixed as a side effect of other changes.
  • We want to detect when the wrong behavior changes to a different flavor of wrongness, as a side effect of other changes.
  • If a user of the software package discovers something that looks broken, they can look into the tests to find the behavior documented, and marked as a "known issue", with link to the issue queue.
  • The git history will reveal more information about the difference in behavior before and after the fix. It will also reveal in which version the problem was fixed.
Improve this question
18
  • 1
    IMO, if one of the roles associated with the tests is to describe behavior, then they should describe the correct behavior; however if you're refactoring, you shouldn't change the overall behavior of the system, just the structure and the way different components (old or newly created) interact. So you can write a temporary "safety net" test that allows you to do the restructuring first, to get you to a point where you can begin to understand what the hell is going on in the code - this sort of test could do something "stupid" like concat all the output in a string, and then check 1/2
    – Filip Milovanović
    Commented Jan 14, 2022 at 8:24
  • 1
    ... if you get the same string when you shuffle code around (so, you're not even thinking what the behavior really means). Once you get to a point where you have established concepts (workable methods/classes) in your code that you can reason about on some level, you can write tests that describe (or, I guess, prescribe) the correct behavior for those, one at a time (probably not a good idea to do it all at once), and bring the system into shape bit by bit. Eventually throw away the ad hoc "safety net" test. 2/2
    – Filip Milovanović
    Commented Jan 14, 2022 at 8:24
  • 2
    There is a podcast by Brian Okken where he and a guest discuss a feature called xfail (tests which are known to fail) share.fireside.fm/episode/DOAjrBV2+iHYGDqvh . The podcast is about pytest but as far as I remember they also discuss quite generally the question if failing tests may or shouldn’t be merged into the main line.
    – Hartmut Braun
    Commented Jan 14, 2022 at 12:22
  • 1
    @DavidCary Relevant to "should I merge a failing test?": softwareengineering.stackexchange.com/questions/201743/… - Overall consensus seems no.
    – donquixote
    Commented Jan 14, 2022 at 20:10
  • 1
    @HartmutBraun Very nice episode! xfaiil seems a good idea, but then the test won't describe in detail the current behavior. We want to detect if the buggy behavior changes over time, even if it is still wrong, but in a different flavor of wrong.
    – donquixote
    Commented Jan 15, 2022 at 0:44

3 Answers 3

Reset to default
6

Add the tests at the same time that you fix the bug (your option 4). The final tests would then describe the desired, non-buggy behavior because they would pass. This is pretty typical for bug-fixing.

Unless you're dealing with a massive bug, I can't see much reason to spend effort writing and merging tests that you know are incorrect, only to have to come back at some point in the future and update those tests again. There's also the possibility that you look at the passing tests and close the bug ticket as OBE or not reproducible or some such - maybe not likely, sure, but why chance it.

Edit to clarify: you can certainly write the tests with the current, buggy behavior; fix the bug; and update the tests to be sure you're not breaking anything else (although those other things have tests, too, right?). But don't spend the effort merging tests with incorrect expected results into your main branch. And definitely don't merge failing tests into your main branch!

Improve this answer
2
  • When you say "wrong tests" do you mean failing tests or tests with "wrong" expectations?
    – donquixote
    Commented Jan 14, 2022 at 3:23
  • 2
    @donquixote You should never merge failing tests to your main branch, but I was referring here to tests with "wrong" expectations. Updated the answer to make that more clear
    – mmathis
    Commented Jan 14, 2022 at 3:26
4

Short answer: They can do both.

Slightly longer short answer:

  • Regression Tests describe the current system,
  • Progression Tests describe the target system.

Long Answer:

Option 1, is called Progressive Tests. More useful at the end to end/integration testing/use case level these tests serve as goals to be achieved. Its understandable that these tests are failing for a long time, and the QA part of the build pipeline should be aware of these. ie. no build with a failing progressive test goes to production, but it can go for local testing.

Option 2. Is refactoring. It is particularly good in areas of code that have descended into murkyness. Write a suite of tests, and (very importantly) refactor the code back into legibility, before doing any code fixes.

Option 3. ... Its horrible... Please don't do this. How are you supposed to know in which "special" mode the test is working. How many times do you have to re-run the test to ensure each of its modes works. How will you even known to re-run the test? If there is an anti-pattern here this is it.

But i could be miss reading you, see option 5 below.

Option 4 is what general development is all about. Make the changes to the code and to the tests necessary to describe the behavior change. When it is working check it in.

There is an option 5:

Option 5. It's useful when you want to preserve old behaviour even for a short amount of time. Write separate tests for each behaviour in the different supported modes. Each test is responsible for configuring the feature toggles for setting up the mode it is testing.

Should you no longer support a mode, delete the tests and the code behind the toggle. Simple.

Improve this answer
7
  • "Option 3. ... Its horrible... But i could be miss reading you". In a test, I might write if (knownIssueExists()) { doThis(); } else { doThat(); }. Later, when fixing the bug, we replace the entire if/else with just ` doThat();. There could be a cli/env param to make knownIssueExists()` return FALSE - but by default, or in a CI pipeline, it would return TRUE, and perhaps write to a log somewhere to turn the result yellow (not red).
    – donquixote
    Commented Jan 14, 2022 at 21:35
  • More advanced, we could have different bugs represented as classes or methods, and then if (KnowIssue12345::exists()) { assertThis(); } else { assertThat(); }. Then a developer could look into the known issues directory and find documented bugs. When fixing a bug, they would delete the respective class and all references to it, leaving only assertThat();. Later you could look into the git history and find when exactly a documented bug was fixed.
    – donquixote
    Commented Jan 14, 2022 at 21:38
  • Tbh, I have not seen this kind of stuff anywhere else, and have not really thought it through to the end. It seems in most projects this would be a rare case anyway, because most bugs would be fixed directly. A simple // @todo comment might be good enough for the remaining cases.
    – donquixote
    Commented Jan 14, 2022 at 21:41
  • Btw.. seems like a "Progression Test" becomes a "Regression Test" once the feature is implemented.
    – donquixote
    Commented Jan 14, 2022 at 23:38
  • Refined version of the above: try { self::assertDesired(); self::fail('Fixed, remove this'); } catch (AssertionFail $e) { KnownBug12345::report($e); self::assertActualBadBehavior(); }. The reference to KnownBug12345 can be used for "find usages" search.
    – donquixote
    Commented Jan 15, 2022 at 1:05
3

Most times I agree with mmathis. Submit the updated tests along with the bug fix. There are cases where you might find a defect, but don't have time to fix it, or fixing that defect is out of scope for your current task. Not every test needs to pass or fail. Many test frameworks come with a third outcome called "inconclusive", "pending" or some similar name — basically a test that neither passes, nor fails.

I work mostly in .NET using MS Test. I have written failing unit tests before, but instead of letting it fail, I call Assert.Inconclusive("should begin passing when bug #1234 is fixed") at the beginning of the test. A quick search for PHPUnit seems like $this->markTestIncomplete("should begin passing when bug #1234 is fixed") might be a good choice.

This usually doesn't fail the build, but the test run might not appear "green", nor will it appear "red". When encountering these "in between" test outcomes, some test runners will report that the tests did not pass and did not fail. The test run would be appear "yellow". This might require some explanation to teammates or management.

The real challenge is to eventually fix the application, and get that test running and passing, but in the meantime at least you have a test that will fail if you comment out a line of code. That gives the future bug fixer an easy way to jump into correcting the code.

Improve this answer
5
  • Hm. With "->markTestIncomplete()", further assertions in that test won't be reached, even if these might cover valid behavior. E.g. perhaps the bug is about a string output being wrong in some cases, but the rest is still ok. Would be nice to have something that just logs or prints the warning, instead of completely failing. Or something where you can pass a cli param to switch to a different mode.
    – donquixote
    Commented Jan 14, 2022 at 4:16
  • 1
    Some test frameworks also have an "expected to fail" annotation that you can use on a testcase. That way, you can merge the testcase with the correct expected behavior in it before the bug is fixed and without breaking the mainline builds.
    – Bart van Ingen Schenau
    Commented Jan 14, 2022 at 7:18
  • @donquixote: in a case like this, I would advise not making multiple assertions in a single test. Ideally it should be single test, single assertion. Especially in this situation.
    – Greg Burghardt
    Commented Jan 14, 2022 at 12:26
  • And focusing on a single assertion allows you to give that failing test a good name, which can be just as important as writing the test in the first place. This is an opportunity to better describe the situation.
    – Greg Burghardt
    Commented Jan 14, 2022 at 12:30
  • @GregBurghardt If a test case simulates a process with multiple steps, it seems a good idea to have multiple assertions along the way. Or it can build up a "log" of different observations, and later assert the contents of the log against an expected log. The bug might only concern one part of this process, e.g. a wrong message being shown somewhere. If we ant to split this up, we have to faithfully simulate the intermediate state before each step of the process.
    – donquixote
    Commented Jan 14, 2022 at 21:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.