This sounds like a great idea to me. -Darin
On Mon, Jan 5, 2009 at 3:25 PM, Pam Greene <[email protected]> wrote: > We don't have very good unit test coverage (in the broad sense, including > ui_tests, test_shell_tests, etc.) for our code. We've always had a policy > that any new code had to have an associated test, but historically we've > been really bad about enforcing it. > > As a way to help contributors and reviewers remember to add tests, here's a > proposal to have gcl report when they are (or might be) missing. It's a > very rough guess now, and will probably need some refinement as we see what > it misses and where its false positives are too annoying. > > What changes might need tests? > > - Any new source file (.cc, .cpp, .m, or .mm), or > - Any new method added to a source or header (.h file > - A new method is identified by a flush-left non-comment line that > has ( somewhere before the next flush-left line and either ends with { > or > has { at the start of the next flush-left line. > > What counts as a test? > > - Any change to any code file whose name ends in test.* or tests.* > - This is very rough, but at least it shows that the contributor > thought about testing when making the patch > > What do we do if we don't find a test? > > - On 'gcl change', report a warning to the user > - On 'gcl upload', add a warning to the change description so the > reviewer sees it too > - Add an option to override this > > Future possibilities > > - Is it worth restricting the check to only public or protected > methods? > - Since any "real" change ought to either fix a bug or add a feature > that should be tested, warn whenever there are no changes to any tests > (including layout tests) or a test_lists file, even when no source files or > methods were added. Alas, this is probably not feasible since we don't > keep > layout tests in the same repository > - Rather than adding a warning to the change description, it'd be nice > to have a separate warning in the review app, so it showed up no matter > what > and the path author didn't have to override anything. But since we want > the > warning in client-side 'gcl change' anyway, for now we'll keep it simple > and > trust people. > > > Comments and volunteers welcome. > > - Pam > > > > > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
