[Twisted-Python] How to say "reverted". (was Re: [Twisted-commits] r22628 - Revert r22624: regression in test_conch.)
On Wed, Feb 20, 2008 at 8:41 PM, Thomas Herve <therve@wolfwood.twistedmatrix.com> wrote:
Author: therve Date: Wed Feb 20 02:41:55 2008 New Revision: 22628
Removed: trunk/twisted/conch/test/test_transport.py Modified: trunk/twisted/conch/manhole_ssh.py trunk/twisted/conch/ssh/factory.py trunk/twisted/conch/ssh/service.py trunk/twisted/conch/ssh/transport.py trunk/twisted/conch/ssh/userauth.py trunk/twisted/conch/test/test_conch.py trunk/twisted/conch/test/test_ssh.py
Log: Revert r22624: regression in test_conch.
Refs #2678
Hello all, Thanks to Thomas for being alert enough to spot this regression and energetic enough to revert it. I'm very glad that we have people[1] monitoring the test suite making sure that we can trust trunk. However, I have two suggestions on how we can announce trunk reverts better. I'm not picking on him in particular, this is just the first revert for which I am detached enough to comment sensibly. 1. When reverting a commit to trunk, the commit message should explain what the regression is. The word 'regression' is used sometimes to mean 'test suite failure' and other times to mean 'a feature that I liked works differently now' or 'this is slower than it was'. If it's a test failure, it's useful to know what test, and particularly whether or not the test was related to the change. If it's not a test failure, it's good to know why the "regression" is considered severe enough to back out the change, rather than just fixing it in place. 2. Reverting someone's contribution is bad news for them. We should break the bad news gently. Backing out someone's changes can often send an unintended message of blame, when we actually want to be encouraging people to contribute. "Revert <revno>: regression in <file>." is terse, unspecific and leaves too much unsaid. We can't do anything about the bad news, but we can change the way we break it. Being more specific helps a lot, as does describing what happens next (e.g. "I'll fix it up and land it for you", "Can you please investigate the failure and fix the test, I'll review the fix for you as soon as it's ready.") jml [1] I'll be even happier when we have *machines* monitoring the test suite, but that's another email.
On Thu, 21 Feb 2008 09:54:05 +1100, Jonathan Lange <jml@mumak.net> wrote:
[snip]
1. When reverting a commit to trunk, the commit message should explain what the regression is.
The word 'regression' is used sometimes to mean 'test suite failure' and other times to mean 'a feature that I liked works differently now' or 'this is slower than it was'. If it's a test failure, it's useful to know what test, and particularly whether or not the test was related to the change. If it's not a test failure, it's good to know why the "regression" is considered severe enough to back out the change, rather than just fixing it in place.
Yes, please.
2. Reverting someone's contribution is bad news for them. We should break the bad news gently.
Backing out someone's changes can often send an unintended message of blame, when we actually want to be encouraging people to contribute. "Revert <revno>: regression in <file>." is terse, unspecific and leaves too much unsaid. We can't do anything about the bad news, but we can change the way we break it. Being more specific helps a lot, as does describing what happens next (e.g. "I'll fix it up and land it for you", "Can you please investigate the failure and fix the test, I'll review the fix for you as soon as it's ready.")
Sounds great. Of much less significance, but related and most people probably don't know about it, "Reopens #1234" in the revert commit message will take care of adding the commit message to the ticket and re-opening it. Jean-Paul
Hello all,
Thanks to Thomas for being alert enough to spot this regression and energetic enough to revert it. I'm very glad that we have people[1] monitoring the test suite making sure that we can trust trunk. However, I have two suggestions on how we can announce trunk reverts better. I'm not picking on him in particular, this is just the first revert for which I am detached enough to comment sensibly.
1. When reverting a commit to trunk, the commit message should explain what the regression is.
The word 'regression' is used sometimes to mean 'test suite failure' and other times to mean 'a feature that I liked works differently now' or 'this is slower than it was'. If it's a test failure, it's useful to know what test, and particularly whether or not the test was related to the change. If it's not a test failure, it's good to know why the "regression" is considered severe enough to back out the change, rather than just fixing it in place.
2. Reverting someone's contribution is bad news for them. We should break the bad news gently.
Backing out someone's changes can often send an unintended message of blame, when we actually want to be encouraging people to contribute. "Revert <revno>: regression in <file>." is terse, unspecific and leaves too much unsaid. We can't do anything about the bad news, but we can change the way we break it. Being more specific helps a lot, as does describing what happens next (e.g. "I'll fix it up and land it for you", "Can you please investigate the failure and fix the test, I'll review the fix for you as soon as it's ready.")
Thanks for insisting on that. I totally agree with you, but of course lazyness sometimes comes in the middle of the way. FWIW, I've contacted Paul before the revert, so I hope he did take it personaly :). As a matter of fact, I think the responsability of revert is the one of the reviewer, because it's the reviewer who gave an early go for the commit. Thanks also to JP for the 'Reopens' feature. I've documented it on the ReviewProcess page, don't hesitate to modify it.
[1] I'll be even happier when we have *machines* monitoring the test suite, but that's another email.
Currently, our test suite is a little bit too unreliable to do this (aka 'intermittent failures'). But maybe in a near future... -- Thomas
Thomas Hervé wrote: [...]
[1] I'll be even happier when we have *machines* monitoring the test suite, but that's another email.
Currently, our test suite is a little bit too unreliable to do this (aka 'intermittent failures'). But maybe in a near future...
We've had intermittent failures for *years*. I doubt that'll get solved in a near future without some sort of significant change to how we do things. Here's a thought experiment: one possibility would be completely automate the running of the test suite, as jml is suggesting, e.g. by using a commit hook (or a “commit bot” like PQM) that will run the test suite before allowing a commit to trunk. Then intermittent failures become much more intolerable, because they will regularly frustrate developers rather than being something you can usually ignore. Thus people will be much more motivated to find solutions to our intermittent tests (whether by fixing the offending tests, or disabling them, or something else). I'm not sure it'd be worth the effort, but it's interesting to think about... -Andrew.
Quoting Andrew Bennetts <andrew-twisted@puzzling.org>:
Thomas Hervé wrote: [...]
[1] I'll be even happier when we have *machines* monitoring the test suite, but that's another email.
Currently, our test suite is a little bit too unreliable to do this (aka 'intermittent failures'). But maybe in a near future...
We've had intermittent failures for *years*. I doubt that'll get solved in a near future without some sort of significant change to how we do things.
Here's a thought experiment: one possibility would be completely automate the running of the test suite, as jml is suggesting, e.g. by using a commit hook (or a ?commit bot? like PQM) that will run the test suite before allowing a commit to trunk. Then intermittent failures become much more intolerable, because they will regularly frustrate developers rather than being something you can usually ignore. Thus people will be much more motivated to find solutions to our intermittent tests (whether by fixing the offending tests, or disabling them, or something else).
I'm not sure it'd be worth the effort, but it's interesting to think about...
I think it definitely worth the effort. As I see it, maybe we could split the effort in 2 parts: * having one commit bot for our best supported platform. I think debian-py2.4-select has been free of intermittent failure for a bit now. Maybe we could trust it enough to disallow commits that breaks it. The other advantage for this is that we could formalize the merge process a bit more: for example, with a web page where you specify the branch name, authors, reviewers, tickets fixed, etc. It looks like what PQM is doing. * having better notifications for trunk commits that generate failed runs. Today, this work is basically done by JP looking at the buildbot and remembering the failures and tickets linked to them, and possibly creating a new ticket if it's not an already identified problem. It's not only cumbersome but probably let some errors pass through. We could look at this after the release :). -- Thomas
participants (4)
-
Andrew Bennetts -
Jean-Paul Calderone -
Jonathan Lange -
Thomas Hervé