TEST= field in the changelists

[Alexander Potapenko]

Hi team,

I often notice that people add a "TEST=" line to their changelists
that fix user-invisible bugs like compilation failures, Valgrind
reports or perf regressions.
However our presubmit scripts suggest to add this field if the change
"requires manual test instructions to QA team".

What's the recommended practice of using TEST=?
Is using it in every CL necessary?

Alex

[Nico Weber]

See http://groups.google.com/group/chromium-dev/browse_thread/thread/61f00c56187a36f9
for the thread that started the TEST= line.

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
>

[Scott Hess]

On Thu, Oct 20, 2011 at 4:35 AM, Alexander Potapenko
<gli...@chromium.org> wrote:

If you don't have a TEST= line, does that mean you don't need tests,
or does it mean that you forgot to add the TEST= line?

TEST=none means that you don't need tests. I've also seen
TEST=waterfall , which seems appropriate for changes which target
waterfall feedback.

-scott

[John Abd-El-Malek]

For the record, I'm really against

TEST=

BUG=

or

TEST=none

BUG=none

If there's nothing to say to the test team, or no associated bug then no need to explicitly call that. We already have a presubmit check that tells developers to add them if there's instructions/bug if they're missing.

[Don Garrett]

The ChromeOS presubmit checks require those fields to be present no
matter what, so the habit is to use 'None' values if there is nothing
interesting to put there. Would it be useful to the testing teams if
this changed?

--
Don

[John Abd-El-Malek]

Given that the chrome ones don't require them, I'm not sure how it helps the test team if only a subset of code requires them?

[Peter Kasting]

On Thu, Oct 20, 2011 at 11:05 AM, Scott Hess <sh...@chromium.org> wrote:

On Thu, Oct 20, 2011 at 4:35 AM, Alexander Potapenko> What's the recommended practice of using TEST=?

> Is using it in every CL necessary?

If you don't have a TEST= line, does that mean you don't need tests,
or does it mean that you forgot to add the TEST= line?

Precisely.  This is why I strongly disagree with John.  Every CL I write should have BUG= and TEST=, either with "none" or actual content.

PK

[Mark Mentovai]

TEST= isn’t just meant for the QA team. Its primary purpose is to make
sure that you, the developer, have thought about how to test what
you’re changing and actually performed that test. Sometimes TEST= will
be the name of a test in some test suite, like “TEST=base_unittests
FilePathTest.StripTrailingSeparators”. Sometimes it’ll be a set of
reproduction steps with expectations. Some changes aren’t really
testable, and that’s fine, but the point of requiring the line is to
make sure that you’ve thought about how to test things.

For compilation failures or performance regressions, the TEST= might
be obvious from the rest of the checkin message, so “TEST=green” or
“TEST=perf graph” might seem redundant. One thing I would like to see,
and that I don’t think people are good enough at doing, is explicitly
stating which checkin caused the problem, which build failed, and
which performance test regressed. Ideally, the checkin message should
contain something that points to a specific buildbot build that
failed. The person who has to unravel your work tomorrow thanks you.

I think BUG= lines are even more important than TEST=, because we
track so many things by bugs. Really, any nontrivial change should
have a BUG=. If you’re fixing a build or performance regression, it’s
OK to recycle the BUG= from the original change that caused the
problem. This helps tie changes together and helps figure out if
subsequent fixes need to be merged to other branches when the original
change was. It also provides a place to set blocking and for
post-commit discussion if a change breaks anything or people have
other concerns. New bugs are cheap and easy to file, and don’t need
much new information—it’s OK to reuse your commit message. Often you
can reuse an existing bug number or file a single bug for related work
that spans multiple check-ins.

[John Abd-El-Malek]

I think the difference in opinion is because I trust that the average developer will add a BUG if there's a bug, and  TEST if there's instructions to the test team or to other developers looking at this. The presubmit checks remind people to do this precisely at upload  time. If people don't read presubmit checks, then we should remove them all since they're slowing us down for no reason. If people do read it, then I don't see why they need to confirm that they read it in the cl desc.

--

[Ojan Vafai]

The TEST= line was originally added *exclusively* as instructions to QA as to how to test changes. See the thread Nico linked to. It was added because QA was having difficulty verifying the correctness of many user-facing patches.

If the meaning of that line has changed since, I don't remember seeing an email to that effect. I'm opposed to having boilerplate saying you've actually run the tests. You're expected to run the tests for code you change. You shouldn't need to clutter commit descriptions claiming you have run them. Also, I imagine this detracts from the original purpose of the line. People think that if they list the tests they've run that they've fulfilled the purpose of the line even if they haven't given any QA instructions as to how to manually test it.

[Jonathan Kliegman]

(I mainly work on Chromium-OS so my view may be slightly different and its possibly CrOS has a different policy).

From submitting CL's and responding to reviewer feedback I've always treated the TEST= line as a list of what tests were run.  And given that occasionally a CL of mine has a problem or unintended side effect I often notice a few months later, being able to go back and see how I actually tested the bug really helps me in tracking down regression.  

For example, I made a change to a value in make.conf to enable a feature that wasn't ready to be used. ~1 month later when I tried to use that feature it didn't work.  Looking back at my original CL and the test cases I ran it was easy to see why it wasn't working (the tests I'd ran only checked for inclusion of a new value being propagated and didn't realize that the old values got clobbered).  If I didn't have this, I'd have spent a lot more time trying to track down and see why it worked when I checked it in but didn't work now.  

It also seems to me like directions to verify/fix belong in the bug, not the CL.  QA is testing based on bugs, I doubt they are trying to individually verify every CL that goes in.  So make sure instructions to test/reproduce/verify go in the bug and the individual CL's can indicate the tests ran to verify the bug (and simple responses like 'trybots', 'suiteSmoke' or similar is probably sufficient).

[Anantha Keesara]

Please see comments liline

On Mon, Oct 31, 2011 at 7:55 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

(I mainly work on Chromium-OS so my view may be slightly different and its possibly CrOS has a different policy).

From submitting CL's and responding to reviewer feedback I've always treated the TEST= line as a list of what tests were run.  And given that occasionally a CL of mine has a problem or unintended side effect I often notice a few months later, being able to go back and see how I actually tested the bug really helps me in tracking down regression.  

For example, I made a change to a value in make.conf to enable a feature that wasn't ready to be used. ~1 month later when I tried to use that feature it didn't work.  Looking back at my original CL and the test cases I ran it was easy to see why it wasn't working (the tests I'd ran only checked for inclusion of a new value being propagated and didn't realize that the old values got clobbered).  If I didn't have this, I'd have spent a lot more time trying to track down and see why it worked when I checked it in but didn't work now.  

It also seems to me like directions to verify/fix belong in the bug, not the CL.  QA is testing based on bugs, I doubt they are trying to individually verify every CL that goes in.  

QA has a tool that parses the CL, for every build , and lists all the bugs that have TEST=.  QA then verifies those bug fixes(which have steps). TEST= some times have info like... Look at bug for instructions... This is also very helpful. So please use TEST=.

Thanks,

Anantha.

[Jonathan Kliegman]

On Mon, Oct 31, 2011 at 11:18 AM, Anantha Keesara <ana...@chromium.org> wrote:

Please see comments liline

On Mon, Oct 31, 2011 at 7:55 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

(I mainly work on Chromium-OS so my view may be slightly different and its possibly CrOS has a different policy).

From submitting CL's and responding to reviewer feedback I've always treated the TEST= line as a list of what tests were run.  And given that occasionally a CL of mine has a problem or unintended side effect I often notice a few months later, being able to go back and see how I actually tested the bug really helps me in tracking down regression.  

For example, I made a change to a value in make.conf to enable a feature that wasn't ready to be used. ~1 month later when I tried to use that feature it didn't work.  Looking back at my original CL and the test cases I ran it was easy to see why it wasn't working (the tests I'd ran only checked for inclusion of a new value being propagated and didn't realize that the old values got clobbered).  If I didn't have this, I'd have spent a lot more time trying to track down and see why it worked when I checked it in but didn't work now.  

It also seems to me like directions to verify/fix belong in the bug, not the CL.  QA is testing based on bugs, I doubt they are trying to individually verify every CL that goes in.  

QA has a tool that parses the CL, for every build , and lists all the bugs that have TEST=.  QA then verifies those bug fixes(which have steps). TEST= some times have info like... Look at bug for instructions... This is also very helpful. So please use TEST=.

They list all the bugs that have TEST=?  Or all CL's that have TEST=?

Sine bugs already have BUG=???? which links to bugs, shouldn't the data on verifying the fix be in the bug?

[Ojan Vafai]

On Mon, Oct 31, 2011 at 8:28 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 11:18 AM, Anantha Keesara <ana...@chromium.org> wrote:

Please see comments liline

On Mon, Oct 31, 2011 at 7:55 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

(I mainly work on Chromium-OS so my view may be slightly different and its possibly CrOS has a different policy).

From submitting CL's and responding to reviewer feedback I've always treated the TEST= line as a list of what tests were run.  And given that occasionally a CL of mine has a problem or unintended side effect I often notice a few months later, being able to go back and see how I actually tested the bug really helps me in tracking down regression.  

For example, I made a change to a value in make.conf to enable a feature that wasn't ready to be used. ~1 month later when I tried to use that feature it didn't work.  Looking back at my original CL and the test cases I ran it was easy to see why it wasn't working (the tests I'd ran only checked for inclusion of a new value being propagated and didn't realize that the old values got clobbered).  If I didn't have this, I'd have spent a lot more time trying to track down and see why it worked when I checked it in but didn't work now.  

It also seems to me like directions to verify/fix belong in the bug, not the CL.  QA is testing based on bugs, I doubt they are trying to individually verify every CL that goes in.  

QA has a tool that parses the CL, for every build , and lists all the bugs that have TEST=.  QA then verifies those bug fixes(which have steps). TEST= some times have info like... Look at bug for instructions... This is also very helpful. So please use TEST=.

They list all the bugs that have TEST=?  Or all CL's that have TEST=?

Sine bugs already have BUG=???? which links to bugs, shouldn't the data on verifying the fix be in the bug?

This is the whole point of the TEST= line. There are many bugs where it's not clear to QA how to verify correctness (e.g. the bug is hard to reproduce or the language in the bug is just vague). 

IMO, we should rename it to QA-INSTRUCTIONS and then there would be no confusion.

[James Hawkins]

On Mon, Oct 31, 2011 at 8:36 AM, Ojan Vafai <oj...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 8:28 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 11:18 AM, Anantha Keesara <ana...@chromium.org> wrote:

Please see comments liline

On Mon, Oct 31, 2011 at 7:55 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

(I mainly work on Chromium-OS so my view may be slightly different and its possibly CrOS has a different policy).

From submitting CL's and responding to reviewer feedback I've always treated the TEST= line as a list of what tests were run.  And given that occasionally a CL of mine has a problem or unintended side effect I often notice a few months later, being able to go back and see how I actually tested the bug really helps me in tracking down regression.  

For example, I made a change to a value in make.conf to enable a feature that wasn't ready to be used. ~1 month later when I tried to use that feature it didn't work.  Looking back at my original CL and the test cases I ran it was easy to see why it wasn't working (the tests I'd ran only checked for inclusion of a new value being propagated and didn't realize that the old values got clobbered).  If I didn't have this, I'd have spent a lot more time trying to track down and see why it worked when I checked it in but didn't work now.  

It also seems to me like directions to verify/fix belong in the bug, not the CL.  QA is testing based on bugs, I doubt they are trying to individually verify every CL that goes in.  

QA has a tool that parses the CL, for every build , and lists all the bugs that have TEST=.  QA then verifies those bug fixes(which have steps). TEST= some times have info like... Look at bug for instructions... This is also very helpful. So please use TEST=.

They list all the bugs that have TEST=?  Or all CL's that have TEST=?

Sine bugs already have BUG=???? which links to bugs, shouldn't the data on verifying the fix be in the bug?

This is the whole point of the TEST= line. There are many bugs where it's not clear to QA how to verify correctness (e.g. the bug is hard to reproduce or the language in the bug is just vague). 

IMO, we should rename it to QA-INSTRUCTIONS and then there would be no confusion.

 

Or a shorter version, REPRO=.  I personally thought the TEST= line was for tests that you *added* in the CL that verified the fix.  TEST=none was a sign for the reviewer to ask, "Are there reasons why you didn't add a test?"  However, it seems TEST= means quite a few things to different people.

James

[Dominic Mazzoni]

On Mon, Oct 31, 2011 at 8:36 AM, Ojan Vafai <oj...@chromium.org> wrote:

IMO, we should rename it to QA-INSTRUCTIONS and then there would be no confusion.

+1. It will be really hard to change the interpretation of TEST now, I'm pretty sure the majority of the team is using it incorrectly.

Also, can we just modify gcl and git-cl so that they prepopulate new changelists with BUG=, TEST=, QA-INSTRUCTIONS=, TBR=, and other possible keywords and let people delete the ones that don't apply, rather than having a presubmit check throw a warning if you don't include BUG= (which is probably what confused everyone in the first place)?

[John Abd-El-Malek]

Then we will have most changes with empty X= lines. I would prefer we don't do this.

 

The presubmit check was added after most people were using it incorrectly, and it hasn't led to a change in behavior unfortunately.

[Jonathan Kliegman]

On Mon, Oct 31, 2011 at 11:36 AM, Ojan Vafai <oj...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 8:28 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 11:18 AM, Anantha Keesara <ana...@chromium.org> wrote:

Please see comments liline

On Mon, Oct 31, 2011 at 7:55 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

(I mainly work on Chromium-OS so my view may be slightly different and its possibly CrOS has a different policy).

From submitting CL's and responding to reviewer feedback I've always treated the TEST= line as a list of what tests were run.  And given that occasionally a CL of mine has a problem or unintended side effect I often notice a few months later, being able to go back and see how I actually tested the bug really helps me in tracking down regression.  

For example, I made a change to a value in make.conf to enable a feature that wasn't ready to be used. ~1 month later when I tried to use that feature it didn't work.  Looking back at my original CL and the test cases I ran it was easy to see why it wasn't working (the tests I'd ran only checked for inclusion of a new value being propagated and didn't realize that the old values got clobbered).  If I didn't have this, I'd have spent a lot more time trying to track down and see why it worked when I checked it in but didn't work now.  

It also seems to me like directions to verify/fix belong in the bug, not the CL.  QA is testing based on bugs, I doubt they are trying to individually verify every CL that goes in.  

QA has a tool that parses the CL, for every build , and lists all the bugs that have TEST=.  QA then verifies those bug fixes(which have steps). TEST= some times have info like... Look at bug for instructions... This is also very helpful. So please use TEST=.

They list all the bugs that have TEST=?  Or all CL's that have TEST=?

Sine bugs already have BUG=???? which links to bugs, shouldn't the data on verifying the fix be in the bug?

This is the whole point of the TEST= line. There are many bugs where it's not clear to QA how to verify correctness (e.g. the bug is hard to reproduce or the language in the bug is just vague). 

IMO, we should rename it to QA-INSTRUCTIONS and then there would be no confusion.

 

While I don't think its enforceable via git cl or gerrit, would making the bugs require a QA-INSTRUCTIONS or TEST= section be preferable?  It really feels to me like this is the right place for it.  Or is it too likely without reminders/nag notices people will forget to include?

[John Abd-El-Malek]

On Mon, Oct 31, 2011 at 8:58 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 11:36 AM, Ojan Vafai <oj...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 8:28 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 11:18 AM, Anantha Keesara <ana...@chromium.org> wrote:

Please see comments liline

On Mon, Oct 31, 2011 at 7:55 AM, Jonathan Kliegman <kli...@chromium.org> wrote:

(I mainly work on Chromium-OS so my view may be slightly different and its possibly CrOS has a different policy).

From submitting CL's and responding to reviewer feedback I've always treated the TEST= line as a list of what tests were run.  And given that occasionally a CL of mine has a problem or unintended side effect I often notice a few months later, being able to go back and see how I actually tested the bug really helps me in tracking down regression.  

For example, I made a change to a value in make.conf to enable a feature that wasn't ready to be used. ~1 month later when I tried to use that feature it didn't work.  Looking back at my original CL and the test cases I ran it was easy to see why it wasn't working (the tests I'd ran only checked for inclusion of a new value being propagated and didn't realize that the old values got clobbered).  If I didn't have this, I'd have spent a lot more time trying to track down and see why it worked when I checked it in but didn't work now.  

It also seems to me like directions to verify/fix belong in the bug, not the CL.  QA is testing based on bugs, I doubt they are trying to individually verify every CL that goes in.  

QA has a tool that parses the CL, for every build , and lists all the bugs that have TEST=.  QA then verifies those bug fixes(which have steps). TEST= some times have info like... Look at bug for instructions... This is also very helpful. So please use TEST=.

They list all the bugs that have TEST=?  Or all CL's that have TEST=?

Sine bugs already have BUG=???? which links to bugs, shouldn't the data on verifying the fix be in the bug?

This is the whole point of the TEST= line. There are many bugs where it's not clear to QA how to verify correctness (e.g. the bug is hard to reproduce or the language in the bug is just vague). 

IMO, we should rename it to QA-INSTRUCTIONS and then there would be no confusion.

 

While I don't think its enforceable via git cl or gerrit, would making the bugs require a QA-INSTRUCTIONS or TEST= section be preferable?  It really feels to me like this is the right place for it.  Or is it too likely without reminders/nag notices people will forget to include?

Only a small fraction of "bugs" on crbug.com need to give explicit instructions to the test team, so we shouldn't require boilerplate TEST=X lines on all bugs.

[Nico Weber]

Everyone on this thread now knows that TEST= is meant for. Encourage correct use in code reviews, and this thread will have made the world a better place :-)

All the other changes suggested on this thread require changing QA's script and will just add to the confusion.

Nico

[Ojan Vafai]

This same discussion has happened many times. There's too much mail for everyone to read it and there are new people working on the project all the time. Unless QA changing there scripts is terribly complicated, it seems worth doing something  more future-proof.

[Nico Weber]

My only wish: If you end up changing that script, please go with something shorter that QA-INSTRUCTIONS (QA= is e.g. 50% shorter than TEST= :-) ).

Nico

[James Hawkins]

Nico, are you OK with REPRO=?

[Nico Weber]

I don't care too much, but REPRO= doesn't seem all that much clearer that TEST= to me.

[John Abd-El-Malek]

I think changing the name will only add to the confusion. QA will still have to look for both titles, since not everyone will change, and there'll be old changes to look at.

[Ojan Vafai]

lol yeah. My preference is for REPRO.

[Dirk Pranke]

In order to not have to keep citing email threads, and to summarize
things, I have updated the "contributing code" page to reflect what I
understand the consensus of this thread to be (such as there is):

https://sites.google.com/a/chromium.org/dev/developers/contributing-code?pli=1

<quote>
If there's an associated bug file, you should also add a
BUG=bug_number line so they can find the associated bug. Multiple bugs
can be listed, comma separated.

If you have manual test instructions that you want to pass to the test
team so that they can validate the fix, add TEST=how_test_test at the
end of the description. Do not use the TEST= file to indicate that you
added automated tests (or unit tests), or that the existing tests
cover the change.

If there is no bug or test, leave out the lines. Do not create the
lines but leave the values blank or set them to None (i.e., don't use
BUG= or BUG=None).

Example:

Increase the goat teleporter timeout threshold to 100 because the old
value of 10 caused problems for extremely overweight goats. Tests show
that the largest goat in existence should be teleported in 50ms, so...

BUG=31337,2754
TEST=Try loading an overweight goat and confirm the teleporter works.
</quote>

Obviously, we can change TEST= to something else if we reach a
consensus on that. Do people agree that the rest of the wording
reflects the consensus (not that you necessarily agree with it)?

-- Dirk

[Peter Kasting]

On Mon, Oct 31, 2011 at 9:01 AM, John Abd-El-Malek <j...@chromium.org> wrote:

Only a small fraction of "bugs" on crbug.com need to give explicit instructions to the test team, so we shouldn't require boilerplate TEST=X lines on all bugs.

 

I haven't checked but I feel like a huge fraction (40%+) of my changes have test instructions.  Maybe it's just that I work on UI where almost all my changes are user-visible, but I try hard to write appropriate test instructions on any change where it's at all possible to manually test it, and in all other cases I put TEST=none to show that I thought about whether the change is testable.

This is also what I've checked for in my reviews since we added TEST= back in... 2007 or so?  It doesn't seem hard.  I'm confused why you're so opposed to being consistently explicit about this sort of thing.  It's not like we're writing WebKit ChangeLog entries here that have a whole verbose boilerplate format, require merge conflict resolution any time you check in, etc.  This is literally two seconds of typing.  Less if we make the tools auto-prefill TEST=.

On Mon, Oct 31, 2011 at 12:01 PM, Ojan Vafai <oj...@chromium.org> wrote:

lol yeah. My preference is for REPRO.

REPRO is much less clear than TEST.  Please do not change the name to this.

I agree with John that changing the name in general is not a good idea.

On Mon, Oct 31, 2011 at 12:21 PM, Dirk Pranke <dpr...@chromium.org> wrote:

If there is no bug or test, leave out the lines. Do not create the
lines but leave the values blank or set them to None (i.e., don't use
BUG= or BUG=None).

This is certainly not consensus.  Personally I disagree pretty strongly with this.  It's also in conflict with the presubmit check AFAIK.

I have removed it (without a replacement) from the doc.

PK

[John Abd-El-Malek]

On Mon, Oct 31, 2011 at 1:09 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 9:01 AM, John Abd-El-Malek <j...@chromium.org> wrote:

Only a small fraction of "bugs" on crbug.com need to give explicit instructions to the test team, so we shouldn't require boilerplate TEST=X lines on all bugs.

 

I haven't checked but I feel like a huge fraction (40%+) of my changes have test instructions.

Almost all my changes don't have this. Like you say, it depends on which part of the product a developer works on.

 

 Maybe it's just that I work on UI where almost all my changes are user-visible, but I try hard to write appropriate test instructions on any change where it's at all possible to manually test it, and in all other cases I put TEST=none to show that I thought about whether the change is testable.

This is also what I've checked for in my reviews since we added TEST= back in... 2007 or so?  It doesn't seem hard.  I'm confused why you're so opposed to being consistently explicit about this sort of thing.  It's not like we're writing WebKit ChangeLog entries here that have a whole verbose boilerplate format, require merge conflict resolution any time you check in, etc.  This is literally two seconds of typing.  Less if we make the tools auto-prefill TEST=.

Since we all agree TEST= are instructions to test team, I still fail to see the need to tell them to do nothing. 

On Mon, Oct 31, 2011 at 12:01 PM, Ojan Vafai <oj...@chromium.org> wrote:

lol yeah. My preference is for REPRO.

REPRO is much less clear than TEST.  Please do not change the name to this.

I agree with John that changing the name in general is not a good idea.

On Mon, Oct 31, 2011 at 12:21 PM, Dirk Pranke <dpr...@chromium.org> wrote:

If there is no bug or test, leave out the lines. Do not create the
lines but leave the values blank or set them to None (i.e., don't use
BUG= or BUG=None).

This is certainly not consensus.  Personally I disagree pretty strongly with this.  It's also in conflict with the presubmit check AFAIK.

The presubmit checks tells a developer to add it if it's necessary. That's not in disagreement.

[Dirk Pranke]

On Mon, Oct 31, 2011 at 1:09 PM, Peter Kasting <pkas...@chromium.org> wrote:

> On Mon, Oct 31, 2011 at 12:21 PM, Dirk Pranke <dpr...@chromium.org> wrote:
>>
>> If there is no bug or test, leave out the lines. Do not create the
>> lines but leave the values blank or set them to None (i.e., don't use
>> BUG= or BUG=None).
>
> This is certainly not consensus.  Personally I disagree pretty strongly with
> this.  It's also in conflict with the presubmit check AFAIK.
> I have removed it (without a replacement) from the doc.

The presubmit check will warn if you are missing BUG= or TEST= lines,
but not fail. I would prefer it if there was some way to not get these
messages (see also, below).

I am not surprised that you disagree. Perhaps consensus is the wrong
word (majority vote?).

Personally, I agree that BUG=X and TEST=X should always be present,
and that BUG=None should be used if there is no bug associated with
the change, and TEST=bots (or green, or waterfall, or something, or
list of specific automated tests) should be used. I don't think
BUG=<blank> or TEST=<blank> is good. That said, I don't think the
majority agrees with you and I.

However, I don't think deleting this line is the right thing to do. If
we are going to allow these lines to be optional, we should say so. If
we are going to allow TEST=<blank> or TEST=None, we should say what
they should be used for as well.

John, it looks like you've reverted my change completely. Did you do
so because you thought the original text did accurately represent the
consensus (or at least was better written than what I had), or rather
that you'd prefer to leave the original text as-is until a consensus
is reached?

-- Dirk

[John Abd-El-Malek]

The later.

 

-- Dirk

[Peter Kasting]

On Mon, Oct 31, 2011 at 1:16 PM, John Abd-El-Malek <jabde...@google.com> wrote:

This is also what I've checked for in my reviews since we added TEST= back in... 2007 or so?  It doesn't seem hard.  I'm confused why you're so opposed to being consistently explicit about this sort of thing.  It's not like we're writing WebKit ChangeLog entries here that have a whole verbose boilerplate format, require merge conflict resolution any time you check in, etc.  This is literally two seconds of typing.  Less if we make the tools auto-prefill TEST=.

Since we all agree TEST= are instructions to test team, I still fail to see the need to tell them to do nothing.

We're not telling the test team to do nothing.  We're telling the CL reviewers that we thought about how this change could be tested and decided it cannot be tested.  The absence of TEST= is ambiguous, the presence of TEST=none is not.  This is why I enforce that all changes I review have a TEST= line and have done so for years.

On Mon, Oct 31, 2011 at 1:25 PM, Dirk Pranke <dpr...@google.com> wrote:

Personally, I agree that BUG=X and TEST=X should always be present,

and that BUG=None should be used if there is no bug associated with
the change, and TEST=bots (or green, or waterfall, or something, or
list of specific automated tests) should be used. I don't think
BUG=<blank> or TEST=<blank> is good. That said, I don't think the
majority agrees with you and I.

I also disagree that it's even clear that our viewpoint is in the minority.

However, I don't think deleting this line is the right thing to do. If
we are going to allow these lines to be optional, we should say so. If
we are going to allow TEST=<blank> or TEST=None, we should say what
they should be used for as well.

I would be happy to write something but it's not obvious what to write right now.

PK

[Dirk Pranke]

Okay, perhaps you, Peter, and I should attempt to come up with a
different suggestion off-list and report back?

-- Dirk

[Tom Hudson]

On Mon, Oct 31, 2011 at 4:32 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Oct 31, 2011 at 1:16 PM, John Abd-El-Malek <jabde...@google.com> wrote:

This is also what I've checked for in my reviews since we added TEST= back in... 2007 or so?  It doesn't seem hard.  I'm confused why you're so opposed to being consistently explicit about this sort of thing.  It's not like we're writing WebKit ChangeLog entries here that have a whole verbose boilerplate format, require merge conflict resolution any time you check in, etc.  This is literally two seconds of typing.  Less if we make the tools auto-prefill TEST=.

Since we all agree TEST= are instructions to test team, I still fail to see the need to tell them to do nothing.

We're not telling the test team to do nothing.  We're telling the CL reviewers that we thought about how this change could be tested and decided it cannot be tested.  The absence of TEST= is ambiguous, the presence of TEST=none is not.  This is why I enforce that all changes I review have a TEST= line and have done so for years.

OK, I squashed my response earlier, but obviously it's necessary: Nico previously said something to the effect of "Everybody who's read this thread now knows what TEST= is for", but your practice is clearly not the same as his practice, and each of you cites some reason to hold your own practice as normative.

Tom

[John Abd-El-Malek]

I think it's obvious that there are big disagreements here. The test team asked for this to be added long time ago. Since then, some people have used it for another meaning (message to other developers, and not to test team). Now people in the latter camp are wanting to force everyone to adopt their usage. I obviously disagree, and don't think we'll come to consensus. The status quo is each reviewer asks something differently of the code they're looking at, which is not ideal.

[Mark Mentovai]

If nobody other than QA is ever using TEST=, and nobody sees the
utility in mandating TEST= for testable-but-not-by-QA changes (as I
proposed earlier in the thread), then we shouldn’t require TEST=.
Changes that are QA-testable should continue to get TEST= lines. The
presubmit script shouldn’t require TEST=. Many (most?) changes aren’t
directly testable by QA.

Requiring TEST= lines that nobody’s using is busywork.

Requiring TEST= lines only for people to leave them empty or
“TEST=none” is noise.

The above applies only to TEST=, not to BUG=.

[Peter Kasting]

On Mon, Oct 31, 2011 at 1:58 PM, John Abd-El-Malek <j...@chromium.org> wrote:

I think it's obvious that there are big disagreements here. The test team asked for this to be added long time ago. Since then, some people have used it for another meaning (message to other developers, and not to test team). Now people in the latter camp are wanting to force everyone to adopt their usage.

I think you're misunderstanding me, assuming you're claiming I'm in the second camp.

The TEST= field is for the test team.  It is also useful to anyone else who happens to want to test your change and doesn't have the context that the CL author and reviewer have.  It is not a "message to other developers" and in particular not a message to the reviewer (who maybe can figure out how to test the change anyway).

The principle behind my comments is as follows: When you write any change you should think about how it can be manually tested (generally by the test folks, conceivably by other interested parties).  Writing some non-empty TEST= field shows that you did so.  Doing it on every change keeps you in the (good) habit of thinking about it.  Having it present before checkin means your reviewer can double-check that you thought about it.  And having it on the checkin itself means that if a QA person looks at the change for whatever reason they'll know whether and how to test it.

By contrast, not adding the field conveys none of those benefits.  Writing "TEST=" alone is even worse as that looks to me (as someone reading the CL) as if you meant to write something but forgot to do so.

What I am really asking for in my reviews is not that people tell me how to test their change, but that they have thought about how some arbitrary person could test their change.  In that sense I believe I am upholding the spirit of TEST= as it was originally created.

PK

[Dirk Pranke]

Okay, we may have reached a stalemate, but since my other activities
for the afternoon consist of recompiling over and over again, I've
attempted to take another pass at this to summarize different uses and
what different people seem to be advocating.

https://docs.google.com/document/d/1r0wVoYmcAMfDQr5kp43hKVv8fLOY3NlXGHU6aJ4jq0A/edit

Feel free to read that over and see what you think. It's largely an
exercise in pedantry, but if we miraculously can agree on some
improvements, there may end up being value in this.

-- Dirk

[John Abd-El-Malek]

I think there isn't any new stuff in this email, so I feel like we're going in circles. Your email below describes a use case that you think is very important. However as I said in my previous email, this isn't why this tag got added. It was added for the test team. Our bugs should have information on how to repro a problem, and it seems that your use case below is attempting to add redundancy. If we're going to ensure anything, it should be that all of our bugs have repro instructions. There's no need to duplicate instructions.

[Peter Kasting]

On Tue, Nov 1, 2011 at 8:57 AM, John Abd-El-Malek <j...@chromium.org> wrote:

I think there isn't any new stuff in this email, so I feel like we're going in circles.

OK.  Then in the interest of moving on with our lives, I propose that we leave the presubmit check and the Chromium docs as they are and leave usage of TEST= up to author and reviewer discretion.  (Sorry Dirk.)

PK

[Pam Greene]

Change descriptions are often redundant with information in bugs. This is not bad. Their access mechanisms and usage patterns are different.

If a developer is looking through the changelog to figure out what happened to a file to cause a regression, it's hugely inconvenient to have to click through and load bugs for 20 revisions. So we put a concise summary of what's in a patch in its log.

Likewise, if someone from QA -- or anyone else -- is trying to figure out whether something has a test (automated unittest or manual repro) and how to invoke it, it's inconvenient to have to load and read a lengthy bug to pick out the information. So we should continue to put a concise summary of how to test a patch in its log.

TEST=none

  "I thought about it, and as a leading expert on this change, I can't think of a way to test it."

TEST=FooBar.DoesntCrash in unit_tests

  "This shouldn't need manual testing, and shouldn't break invisibly. Hooray!"

TEST=Try to teleport a 100kg goat, and make sure its horns don't turn blue.

  "Attention QA, this will have to be done manually."

- Pam

--