Marnie McCormack wrote:
Hi All,
I've just been looking at our continuous build failures from last night.
Which has led me to https://issues.apache.org/jira/browse/QPID-1339 and the
commit that went in against it last night.
Failures this end may or not be related (currently looking likely), but at
any rate it's over the line in the sand ....
- it is a monster commit and hits lots of different packages/logical areas
of the code base. The one liner in the JIRA which was created one hour
before the commit doesn't shed light on the work. I'm looking through each
change to work out what's the work really encompasses.
- are there tests for the changes/additions ? The JIRA should state how the
changes can be tested by the reviewer.
Historic precedent dictates that non-documented refactoring results in the
author having to document the purpose and design of the refactoring !
Hey Marnie,
First off apologies for breaking the build. I know how frustrating it
can be to have to track down bugs in other people's commits.
Having discussed this particular case a bit on partychat there seem to
be some issues with the current set of test profiles. The commit in
question makes it through 'ant test -Dprofile=cpp' *and* 'ant test' with
no reported failures. The protocol negotiation issue isn't evident until
the client attempts to connect to an external (non VM) 0-8 or 0-9
broker, and unfortunately 'ant test' doesn't ever do this.
At the moment, our major test profiles (cpp, default, and java) are
split up in terms of functional codepath. The cpp profile tests the 0-10
codepath, the default profile tests the 0-8/9 codepath against the in-VM
broker, and the java profile tests the 0-8/9 codepath against an
external broker. This means that to get even basic testing for all
client codepaths, you need to run all 3 profiles. Unfortunately this
takes about 45 minutes for even a reasonably beefy machine to churn
through, so it's quite tempting to take shortcuts if you're reasonably
confident that your code changes don't touch a particular codepath, and
obviously its quite possible to be wrong!
One thing we could do to improve the situation is make the default 'ant
test' run at least some tests against each external broker. This would
guarantee that *all* the codepaths of the client get at least some basic
test coverage from the default set of tests that are easy to run prior
to each commit.
I do have some thoughts about your other concerns regarding the dev
process, but I'll post separately on that topic.
--Rafael
