Hi Tor Arne, That's an excellent summary. I have questions/comments on a few of the early-warning tests (see below), but the philosophy and the high-level model both look good to me.
> Some ideas for tests: ... > - If the patch contains a new test, run the test before applying the > code change, to verify that the test actually fails without the patch Depending on the nature of the patch, the test may fail to compile (or worse, loop infinitely or crash the test environment) when run without the rest of the patch. The test runner will need to be a little bit clever to handle these cases. I'm not sure whether it makes sense to include performance and system-level tests in the early warning system, or just unit tests. Performance tests might be a tricky case to handle automatically, as the test might not fail without the rest of the patch (e.g. if the patch makes performance a little better rather than a lot better), and also because improving performance on one architecture might make it worse on another (e.g. a patch which uses larger but faster code could actually run slower if the larger version of the code doesn't fit in cache on some architectures). Sometimes it's also necessary to sacrifice some performance to get a more correct implementation, so patches that fail performance tests probably need to be reviewed manually rather than being rejected automatically. > - Check that the commit message conforms to the template The current legal checks (for prototype name leakage and names of employees who wish to remain anonymous) will need to be retained for changes that are pushed from inside Nokia. I think those particular checks need to be done earlier though, before external people can see the patch. In a way, we'll need to apply some stricter checks to stuff pushed from within the company than to stuff pushed from outside, due to various legal obligations the company must uphold (e.g. the company is legally forbidden from publishing employee names without their consent). There are also other legal checks that must be applied to all commits, e.g. for terms we can no longer use, such as "multitouch" and "Qt Software". > - Check that there's only one author in the patch-set That restriction seems a little odd to me. Can you explain the reason behind it? I can envisage cases where a new feature is developed by a team, or where I go on vacation after making a patch-set and a second person in my team fixes the last autotest failure so that the patch-set can get through before I return from vacation. I would see the EWS as a means to catch errors that are obvious and/or are easy/cheap to find automatically -- the kind of things that a human reviewer would likely see as wasting their time: - check that there are no new compiler warnings, - check that new classes/functions have docs, - check that there are no new qdoc warnings, - run static checking tools, e.g. Coverity, if it runs fast enough. Finally, I think there needs to be a way for a human arbiter to override the decision of the EWS in the (hopefully rare) cases where the EWS will reject a valid change. -- Jason _______________________________________________ Opengov mailing list Opengov@qt-labs.org http://lists.qt-labs.org/listinfo/opengov
