2

Our small team has made the curious observation that diagnosing and fixing broken automated tests is a useful activity; in particular, if the CI build indicates that developer A's last check-in broke some tests, developer B can learn about (and possibly improve on) what developer A was doing by understanding why the test failed, and fixing it (either the code or the test, as appropriate.) This activity confers some of the benefits normally attributed to pair programming or code review, as far as getting more pairs of eyes on tricky pieces of code.

I'm wondering if other teams have made this same observation, and whether a practice like this makes sense to other people? Should the person who broke it, fix it, or should that job fall to co-workers?

gnat
  • 21,442
  • 29
  • 112
  • 288
Ernest Friedman-Hill
  • 462
  • 3
  • 12
  • 1
    But A ran the tests before they checked their code in and everything was fine! – JohnMark13 Sep 06 '13 at 15:47
  • @JohnMark13 I wish! But the full test suite is too large for that. In any case, I'm seriously proposing that the process of spreading the fixes around is useful, and if no tests ever broke, the utility would be lost. – Ernest Friedman-Hill Sep 06 '13 at 16:14
  • 1
    FYI to openers/closers, closing a question as a duplicate of a question that's on hold and headed for closure ain't quite fair. Voting to re-open. – Ross Patterson Sep 07 '13 at 13:24
  • 1
    @RossPatterson would you rather this question be closed as primarily opinion based? The dup leads to a question that has been seen nearly 3k times, and has 18 answers. Its asking the same question. Someone coming here from google should be directed to the other question (that is what the dup is intended for). –  Sep 07 '13 at 14:22
  • @MichaelT Maybe I misunderstand what `[on hold]` means. If it means "very likely to be closed", then I'd say this question should be re-opened. The OP is asking others to share their experiences with a particular practice, that seems legit to me. – Ross Patterson Sep 07 '13 at 18:03
  • @RossPatterson in that case, it should likely be closed as a polling question instead (either too broad or primarily opinion based). –  Sep 07 '13 at 18:05
  • 2
    I have yet to figure this site out -- it's not clear what sort of questions you can ask here that *aren't* opinion or poll-based. Any answers based on "best practices" are opinion-based by definition. – Ernest Friedman-Hill Sep 07 '13 at 19:19
  • @ErnestFriedman-Hill As with SO, P.SE is concentrated on problems and the answers that solve them. A question on SO of "How many different ways are there to write code to compute the fibonacci sequence" doesn't have any problem and would be closed. Questions need **an** answer - sharing experiences does not have one answer, it has many, and is thus not a good question for P.SE. The key is to write a question that identifies a problem that can be solved with **an** answer. –  Sep 08 '13 at 00:11
  • @MichaelT Are you trying to say that JohnMark13's answer below isn't valuable? It's a fine piece of advice, perfectly worthy of being an answer on Programmers. – Ross Patterson Sep 08 '13 at 01:10
  • @RossPatterson marking a question as a duplicate, or closing a question says nothing about the quality or value of the answers. It says that the structure of the question is one that does not fit within the format expected and may not lead to having an answer that solves the problem defined in the question. If there are further questions on this, I would encourage you to ask it on http://meta.programmers.stackexchange.com –  Sep 08 '13 at 01:32
  • Many of most popular questions here are community wiki, maybe transfer this one too? – grizwako Sep 09 '13 at 09:01

2 Answers2

5

So I was being flippant in my comment, but, that should be the case and not only that there should be a defined process for what happens and how long the build may remain broken for before the code just get rolled straight back out again.

  • If a developer can not build the product before checking their code in, then the project is too big or not modular enough.
  • If developer A changed the API and this broke the tests, the developer A should have seen that coming and it should have been flagged before making the changes.
  • If the full suite of tests is huge and can only run over night, they should be broken down into smaller suites. Usually these would relate to the modules of your project and then further into unit and integration tests.
  • You have CI which is great, is it set to poll your SCM for changes and run some tests on updates?
  • How often is code being checked in, could it be that check ins are too large and you would benefit from more frequent check ins.
  • Forking/Branching can be useful to manage different development streams if your developers are checking in unrelated changes that may break each others code in ways that they could not have anticipated.
  • Whatever the process is it should be rare that B is fixing A's code alone.
  • Always be pragmatic.

I do not believe that spreading the fixes around (in this instance) is useful, you'd benefit more from pair programming around the problem areas in the first place. In essence be proactive not reactive. You can give all the low priority bugs to your new developers and get them familiar with the whole codebase :)

Regarding

and if no tests ever broke, the utility would be lost

That is silly. If what you are saying is that you lack sufficient code coverage to tell if a core function of the code has been subverted and that is why no tests are breaking then yes, they lack utility. The tests should of course break (and if you're practicing TDD they should even start broken), but the point is that they should break (and be fixed, with or without cooperation between A and B) on the developers machine.

Years ago some bloke out of Eastenders wrote some things about it, but there are probably a million newer resources to cite.. Martin Fowler says.

JohnMark13
  • 711
  • 4
  • 7
1

Best thing would be when the team decides in consensus who should fix the broken tests, depending on the indivdual case. This may be sometimes A, sometimes another person B or both together. If B does it alone, it would be nice at least to inform A on how he fixed it. And if there is some kind of trap some other people of the team should be aware of, the one fixes the bug should inform them, too.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565