Convention for "TEST="?

[Rachel Blum]

Do we have any convention for listing tests belonging to multiple suites? I.e. something like TEST=browser_tests.Foo.bar:unit_tests.Bar.Baz? (Or multiple lines, one for each suite?)

Or is the TEST field pretty much entirely free-form? (A quick scan of past CLs seems to indicate it is, but I wanted to verify w/ the list)

Rachel

[Nico Weber]

Fun fact: TEST= lines are intended to let QA know how to manually test
a change, not to list unit tests.

Nico

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

[Stuart Morgan]

The last mega-thread on this line ("TEST= field in the changelists")
wasn't even able to reach consensus on whether or not putting unit
tests in this field was acceptable practice, so I can guarantee we
won't get agreement on the question of how such a thing should be
formatted :)

-Stuart

[Rachel Blum]

Ah. I _thought_ I remembered there was a thread. Must improve search-fu - meanwhile, thank you for pointing me towards it. (And in light of that thread, I'd like to withdraw my question :)

Rachel

[Mihai Parparita]

BTW, http://dev.chromium.org/developers/contributing-code#TOC-Create-a-change-list-CL- discusses TEST= usage.

Mihai

[Peter Kasting]

On Thu, Jul 26, 2012 at 12:53 PM, Stuart Morgan <stuart...@chromium.org> wrote:

The last mega-thread on this line ("TEST= field in the changelists")
wasn't even able to reach consensus on whether or not putting unit
tests in this field was acceptable practice,

Which is curious, considering the history of this field (added at QA's request so that bugs requiring manual verification could get verification instructions) and the fact that our instructions on this haven't changed over time...

PK

[John Bates]

+1. Related question: does QA verify that the TEST instructions pass for every release or once to change the bug from Fixed to Verified or something else?

 

PK

[Ryan Hamilton]

As far as I know, I have never fixed a bug that QA verified.  But I mostly hang out in net/ land.  FWIW, I've used TEST=some_net_unittest frequently.

[Marc-Antoine Ruel]

Pro-tip:

Anyone sending me a CL to "alter" this feature* will likely get a rubberstamp lgtm.

* Note this requires 2 CLs, one to remove the automatic addition in git_cl.py and gcl.py, the other in src/PRESUBMIT.py to "alter" the call to CheckChangeHasTestField().

M-A

[Peter Kasting]

On Thu, Jul 26, 2012 at 4:32 PM, John Bates <jba...@google.com> wrote:

does QA verify that the TEST instructions pass for every release or once to change the bug from Fixed to Verified or something else?

The latter, but sometimes they add it to the test list for the former as well if it's a particularly big bug.

Marc-Antoine, I have no idea what feature alteration you're talking about.

PK 

[James Robinson]

I've removed the autogeneration of TEST= and the PRESUBMIT.py check to verify it, since it seems clear from the past several rounds of discussion that there is no clear convention or general utility to the field for most patches.  Feel free (but not obligated) to add a TEST= line or anything else that you feel is helpful to a patch description.  If a test should get special test attention by QA, you definitely should be putting that in the patch description and making sure that the bug has good information.

- James

--

[Peter Kasting]

On Thu, Jul 26, 2012 at 5:33 PM, James Robinson <jam...@google.com> wrote:

I've removed the autogeneration of TEST= and the PRESUBMIT.py check to verify it, since it seems clear from the past several rounds of discussion that there is no clear convention or general utility to the field for most patches.  Feel free (but not obligated) to add a TEST= line or anything else that you feel is helpful to a patch description.  If a test should get special test attention by QA, you definitely should be putting that in the patch description and making sure that the bug has good information.

This change makes me sad.

Everyone should think about whether and how their change should be manually tested for every patch they write, even if it turns out that manual testing doesn't make sense.  If that still happens without us enforcing a TEST= field, then great, but I am suspicious that it won't.

I'm not going to just pull rank and revert this commit, but I think it's a mistake, and I think the core problem here is that we (the original core team) haven't done a good job of communicating how to properly use BUG= and TEST= to the 1000+ new people on the team since we added those fields way back in the old days, so people take their best guess, and slowly different subteams develop their own disparate cultures.

I took a stab at making the dev.chromium.org docs (linked by Mihai, http://dev.chromium.org/developers/contributing-code#TOC-Create-a-change-list-CL- ) more clear and coherent w.r.t. how to use BUG= and TEST= fields.  These may not be the practices every engineer uses today, but their goal is to be the safest, clearest and best practices for ongoing testing and maintenance of the codebase.

PK

[Thiago Farina]

On Thu, Jul 26, 2012 at 9:01 PM, Peter Kasting <pkas...@google.com> wrote:
> Marc-Antoine, I have no idea what feature alteration you're talking about.
>

I think he is saying that the "TEST=" is added when the commit message
doesn't have it for the first time you upload your patch for review.
Not sure how to explain this better, though. git cl upload popups my
vim editor and add the TEST= when I don't provide one. ;)

--
Thiago

[Rachel Blum]

Stupid question: Why not add one more comment line via PRESUBMIT, saying what TEST= is supposed to contain? Instant dissemination to the entire team.

Rachel

[John Abd-El-Malek]

There isn't consensus to add a TEST line, so making that doc say that you should have TEST= is premature.

[John Abd-El-Malek]

and to be clear, I mean adding it when it's not needed 

[James Robinson]

As good as that sounds, the data doesn't support that in practice.  People do not use TEST= that way and despite several megathreads this is not changing.  I sampled commits from r147950-r148000 (randomly chosen, I just like round numbers, but from this week) and found the following breakdown of TEST=lines on checkins:

5 checkins have TEST= lines that could be interpreted as instructions for QA or something in that spirit.  10 TEST= lines refer just to unit tests, despite the all-caps directive not to do so in http://dev.chromium.org/developers/contributing-code#TOC-Create-a-change-list-CL-. 22 have either no TEST= line at all, "TEST=" on a line by itself, or TEST=none.  TEST=none is the minority.  4 more have something else random in the TEST=line and the rest of the checkins are merge/gitdeps/roll checkins.

The fact is that most people are not doing what you expect people to do, and in fact by a 2:1 ratio people are doing exactly what you suggest they should not do (list unit tests).  I think that if in general people add QA directives in 10% of checkins then they should be perfectly capable of adding it themselves.  There's no use in autogenerating a line that 90% of checkins should delete, especially when people are twice as likely to misuse (in your interpretation) the line if it's already there.

- James

PK

[Peter Kasting]

On Thu, Jul 26, 2012 at 6:24 PM, James Robinson <jam...@google.com> wrote:

People do not use TEST= that way

Yes, I'm aware of this.  Like I said, I think the biggest problem is that we haven't given clear directions.  The old directions on that page were incredibly vague and the last really clear thread about this was back in about 2007.

PK

[James Robinson]

I'm not sure what your criteria for "clear" is, but the last very large thread about this subject started in October 2011 and continued for quite some time.  If you believe that rather time-draining discussion improved the situation (or even better have some data to suggest that it did), I'd be happy to hear about it.  I don't see any evidence that the use of TEST= in our checkins is in line with the directions you prefer for it, and I think we're much better off not putting the field in by default for people to just delete (or far more likely, misuse).

- James

PK

[Peter Kasting]

On Thu, Jul 26, 2012 at 6:36 PM, James Robinson <jam...@google.com> wrote:

If you believe that rather time-draining discussion improved the situation (or even better have some data to suggest that it did), I'd be happy to hear about it.

No, I don't think long discussions improve things, either in that case or in general.  I think what improves things is when we dictate what the right behavior will be and discussion stops.  Discussion, debate, and in general democracy about these sorts of issues is, as you note, nonproductive.

My goal in writing the doc was to convey the consensus we HAD achieved on how these fields were to be used, from the time when we added them many years ago.  IMO those of us who were around then have the responsibility to convey that culture forwards and we dropped the ball.  I was trying to pick that back up and move things forward instead of merely debating pointlessly via email.

I don't know how else we're to preserve team culture if the folks who have been around longer don't consistently record, convey, expect, and enforce it.  I think in general not enough of said folks are willing to demand that the rest of the team actually follow a consistent standard -- WHATEVER that standard is.  Instead too many people want us to get consensus on things before we do anything and thus ultimately we wind up with no consistency of practice, culture, or style.  But I can't force the issue on my own against opposition, so whatever.

PK

[Jeffrey Yasskin]

One data point:

I came from another team within Google where the convention was to
write a "Tested:" section with a list of how the developer tested the
change, often with a list of unit tests. I found this useful because I
could think of how to test the change once, and copy+paste it from
then on during the review, and if a reviewer noticed that I had missed
some relevant tests, they could point that out. I started writing
TEST=<list_of_automated_tests>, but stopped when a reviewer pointed
out that the docs contradicted my habit. I'm not really even sure what
kinds of things I can put in the QA-oriented field, although I could
probably find out by grepping some logs.

Are there QA people who actively want and are using this TEST= field?
Their opinion should perhaps matter more than programmers'.

Jeffrey

[Peter Kasting]

On Thu, Jul 26, 2012 at 6:56 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

Are there QA people who actively want and are using this TEST= field?

Their opinion should perhaps matter more than programmers'.

+CC Anantha, who can probably find the answer to your question.

PK 

[David Yu]

If the bug does not contain steps to reproduce the failure or verify the fix, then having TEST= with directions to verify the fix is helpful. 

It's also helpful to the person sheriffing the build to look at the list of changes such as http://omahaproxy.appspot.com/changelog?old_version=22.0.1215.3&new_version=22.0.1218.0 and see something like

TEST=Manual steps in the bug.

OR

"TEST=Connect to a multi-monitor Windows system and verify that mouse events are correctly injected even to non-primary monitors." from CL 147846

David

--

[Thiago Farina]

On Thu, Jul 26, 2012 at 9:33 PM, James Robinson <jam...@google.com> wrote:
> I've removed the autogeneration of TEST=

Thanks, that made my life easier ;)

It means that I don't have to waste time pressing vim keywords to delete them :)
It also means that, at least for me, the process of uploading a patch
became faster.

And yes, I have used TEST= for unit_tests, ui_unittests,
views_unittests, but also for QA when making functional changes that
has steps to reproduce the patch.

--
Thiago

[Dominic Mazzoni]

I have no opinion on whether it should be part of the default CL template or not, but either way could we just rename the line from TEST to QA? Even though I learned the correct behavior after the last centithread and have been striving to write good TEST lines, the word "TEST" still makes me think of automated tests, not instructions for QA - and clearly I'm not alone. Just renaming it QA would eliminate that ambiguity - virtually everyone would either have a good clue what to write by default, or at least realize they'd better look it up in the docs.

- Dominic

[Raymes Khoury]

> I came from another team within Google where the convention was to
> write a "Tested:" section with a list of how the developer tested the
> change, often with a list of unit tests.

+1
Regardless of QA, from a developers standpoint almost every CL should
be tested somehow. Having a mandatory section in the CL describing how
the CL was tested (or why it doesn't need to be) forces you and your
reviewer to think about that. That's how I've always seen TEST=, which
I realize now was against our documentation. This is also how it was
used when I was making CrOS changes.

> could we just rename the line from TEST to QA

> Just renaming it QA would eliminate that ambiguity - virtually everyone
> would either have a good clue what to write by default, or at least realize
> they'd better look it up in the docs.

I like this suggestion for clearing up the ambiguity.

[Raymes Khoury]

[Ben Goodger (Google)]

There are many things that developers should be doing.

DESIGN_DOC=

MAKES_APPROPRIATE_USE_OF_COMPONENTS=

etc. etc.

They don't all have to be called out in the CL template.

I don't think this thread has any further value. Let's move on.

Thanks,

-Ben