[chromium-dev] Re: Proposal: enforcing unit testing in gcl
This sounds great. I'd like to get a unit test started for browser.cc
etc very soon.
-Ben
On Thu, Jan 8, 2009 at 1:24 PM, Darin Fisher wrote:
> This sounds like a great idea to me.
> -Darin
>
> On Mon, Jan 5, 2009 at 3:25 PM, Pam Greene 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: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
[chromium-dev] Re: Proposal: enforcing unit testing in gcl
This sounds like a great idea to me.
-Darin
On Mon, Jan 5, 2009 at 3:25 PM, Pam Greene 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: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
[chromium-dev] Re: Proposal: enforcing unit testing in gcl
Yes, there's an override option in the proposal below. It'll still give a
warning on 'gcl change', but that's harmless.
- Pam
On Mon, Jan 5, 2009 at 4:08 PM, Ben Goodger (Google) wrote:
>
> I think you also need a way to override this in some instances, since
> there are methods (especially in views/ and the UI) for which there is
> no simple way to "verify" correctness of an implementation in code -
> e.g. a function that paints a series of bitmaps at specific locations.
> (Nor would we really want to invest in one, since visual designs
> evolve continuously over time).
>
> -Ben
>
> On Mon, Jan 5, 2009 at 3:25 PM, Pam Greene 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: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
[chromium-dev] Re: Proposal: enforcing unit testing in gcl
I think you also need a way to override this in some instances, since
there are methods (especially in views/ and the UI) for which there is
no simple way to "verify" correctness of an implementation in code -
e.g. a function that paints a series of bitmaps at specific locations.
(Nor would we really want to invest in one, since visual designs
evolve continuously over time).
-Ben
On Mon, Jan 5, 2009 at 3:25 PM, Pam Greene 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: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
[chromium-dev] Re: Proposal: enforcing unit testing in gcl
Sounds great, Pam!
I think the gcl command line option to submit without a test should be:
gcl upload --i-am-a-test-slacker
Mike
On Mon, Jan 5, 2009 at 3:25 PM, Pam Greene 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: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
[chromium-dev] Re: Proposal: enforcing unit testing in gcl
I think this sounds like an excellent start. We can tweak it if we
notice things not working properly.
Brett
On Mon, Jan 5, 2009 at 3:25 PM, Pam Greene 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: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
