rebaseline-cl: Don't rebaseline expected-MISSING tests.

rebaseline-cl: Don't rebaseline expected-MISSING tests.

Before this CL: If there are tests with [ Missing ] expectations in TestExpectations, then `webkit-patch rebaseline-cl` will download new baselines for those tests. I think this behavior isn't quite what I think we want, since any such tests are probably not directly related to the CL for which new baselines are being downloaded.

After this CL: `webkit-patch rebaseline-cl` will only download baselines for tests that have unexpected results (either unexpectedly mismatching baselines or unexpectedly missing baselines).

BUG=654604
Committed: https://crrev.com/7526dc346a39764341c162734d57e48cea499ea2
Cr-Commit-Position: refs/heads/master@{#429138}

[qyearsley, 2016-10-27 00:43:29]

Description was changed from

==========
rebaseline-cl: Don't rebaseline expected-MISSING tests.

BUG=654604
==========

to

==========
rebaseline-cl: Don't rebaseline expected-MISSING tests.

Reason: If there are tests with [ Missing ] expectations in TestExpectations,
then currently webkit-patch rebaseline-cl might try to rebaseline those, but
that isn't the expected behavior.

BUG=654604
==========

[qyearsley, 2016-10-27 00:43:29]

qyearsley@chromium.org changed reviewers:
+ wkorman@chromium.org

[qyearsley, 2016-10-31 16:54:12]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[qyearsley, 2016-10-31 16:54:23]

qyearsley@chromium.org changed reviewers:
+ dpranke@chromium.org

[commit-bot: I haz the power, 2016-10-31 16:54:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 17:45:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 17:45:49]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-11-01 16:52:14]

Walter or Dirk -- this CL is ready for review (although it's not urgent).

[Dirk Pranke, 2016-11-01 16:56:16]

I'm a bit confused. It looks like this is affecting tests whose actual result
was Missing, but the CL description says that the *expected* result is Missing.
I wouldn't expect the latter to normally be allowed.

Is this just a mistake in the description?

[qyearsley, 2016-11-01 17:02:46]

On 2016/11/01 at 16:56:16, dpranke wrote:
> I'm a bit confused. It looks like this is affecting tests whose actual result
was Missing, but the CL description says that the *expected* result is Missing.
I wouldn't expect the latter to normally be allowed.
> 
> Is this just a mistake in the description?

Currently adding [ Missing ] expectations in TestExpectations is allowed
(although maybe it shouldn't be?) -- recently I ran `webkit-patch rebaseline-cl`
when there were a bunch of [ Missing ] expectations, and it rebaselined all of
those tests.

Examples of recent CLs that added Missing expectations:

  https://codereview.chromium.org/1988313004
  https://codereview.chromium.org/2460843002

Even if this shouldn't be allowed, I think we still probably don't want
`webkit-patch rebaseline-cl` to download baselines for these tests.

[Dirk Pranke, 2016-11-01 17:28:44]

On 2016/11/01 17:02:46, qyearsley wrote:
> On 2016/11/01 at 16:56:16, dpranke wrote:
> > I'm a bit confused. It looks like this is affecting tests whose actual
result
> was Missing, but the CL description says that the *expected* result is
Missing.
> I wouldn't expect the latter to normally be allowed.
> > 
> > Is this just a mistake in the description?
> 
> Currently adding [ Missing ] expectations in TestExpectations is allowed
> (although maybe it shouldn't be?) -- recently I ran `webkit-patch
rebaseline-cl`
> when there were a bunch of [ Missing ] expectations, and it rebaselined all of
> those tests.

I knew I should've been more precise. Yes, you can commit a file containing [
Missing ].
I don't know why you would need to or why we should allow it (most cases are at
best
covered by NeedsRebaseline, I think).

It's unclear to me whether we should download baselines in this situation, since
I don't
know how we end up there :).

[qyearsley, 2016-11-01 18:08:46]

On 2016/11/01 at 17:28:44, dpranke wrote:
> On 2016/11/01 17:02:46, qyearsley wrote:
> > On 2016/11/01 at 16:56:16, dpranke wrote:
> > > I'm a bit confused. It looks like this is affecting tests whose actual
result
> > was Missing, but the CL description says that the *expected* result is
Missing.
> > I wouldn't expect the latter to normally be allowed.
> > > 
> > > Is this just a mistake in the description?
> > 
> > Currently adding [ Missing ] expectations in TestExpectations is allowed
> > (although maybe it shouldn't be?) -- recently I ran `webkit-patch
rebaseline-cl`
> > when there were a bunch of [ Missing ] expectations, and it rebaselined all
of
> > those tests.
> 
> I knew I should've been more precise. Yes, you can commit a file containing [
Missing ].
> I don't know why you would need to or why we should allow it (most cases are
at best
> covered by NeedsRebaseline, I think).
> 
> It's unclear to me whether we should download baselines in this situation,
since I don't
> know how we end up there :).

Do you think it would be better to disallow missing expectations in general?
(Just filed a bug for this, http://crbug.com/661215)

In any case, my opinion about whether we should download new baselines in this
situation is
that we shouldn't, since I think rebaseline-cl should be concerned only with new
baselines
that are related to the CL in question, and baselines for tests that are marked
as Missing
in the expectations file are probably not related to the current CL.

[Dirk Pranke, 2016-11-01 18:53:20]

On 2016/11/01 18:08:46, qyearsley wrote:
> On 2016/11/01 at 17:28:44, dpranke wrote:
> > On 2016/11/01 17:02:46, qyearsley wrote:
> > > On 2016/11/01 at 16:56:16, dpranke wrote:
> > > > I'm a bit confused. It looks like this is affecting tests whose actual
> result
> > > was Missing, but the CL description says that the *expected* result is
> Missing.
> > > I wouldn't expect the latter to normally be allowed.
> > > > 
> > > > Is this just a mistake in the description?
> > > 
> > > Currently adding [ Missing ] expectations in TestExpectations is allowed
> > > (although maybe it shouldn't be?) -- recently I ran `webkit-patch
> rebaseline-cl`
> > > when there were a bunch of [ Missing ] expectations, and it rebaselined
all
> of
> > > those tests.
> > 
> > I knew I should've been more precise. Yes, you can commit a file containing
[
> Missing ].
> > I don't know why you would need to or why we should allow it (most cases are
> at best
> > covered by NeedsRebaseline, I think).
> > 
> > It's unclear to me whether we should download baselines in this situation,
> since I don't
> > know how we end up there :).
> 
> Do you think it would be better to disallow missing expectations in general?
> (Just filed a bug for this, http://crbug.com/661215)

Probably, but I don't have a strong feeling here.

> 
> In any case, my opinion about whether we should download new baselines in this
> situation is
> that we shouldn't, since I think rebaseline-cl should be concerned only with
new
> baselines
> that are related to the CL in question, and baselines for tests that are
marked
> as Missing
> in the expectations file are probably not related to the current CL.

I think that's probably fair.

LGTM.

[Dirk Pranke, 2016-11-01 18:53:59]

But your description is still wrong, I think?

[qyearsley, 2016-11-01 21:09:44]

On 2016/11/01 at 18:53:59, dpranke wrote:
> But your description is still wrong, I think?

Oh, I think my description is confusingly-written -- what I originally meant was
more like

"(1) The current behavior is to rebaseline all tests with missing baselines even
if this is "expected" in TestExpectations, and
 (2) The current behavior isn't what I think I want"

Now updating description to be less confusing.

[qyearsley, 2016-11-01 21:16:54]

Description was changed from

==========
rebaseline-cl: Don't rebaseline expected-MISSING tests.

Reason: If there are tests with [ Missing ] expectations in TestExpectations,
then currently webkit-patch rebaseline-cl might try to rebaseline those, but
that isn't the expected behavior.

BUG=654604
==========

to

==========
rebaseline-cl: Don't rebaseline expected-MISSING tests.

Before this CL: If there are tests with [ Missing ] expectations in
TestExpectations, then `webkit-patch rebaseline-cl` will download new baselines
for those tests. I think this behavior isn't quite what I think we want, since
any such tests are probably not directly related to the CL for which new
baselines are being downloaded.

After this CL: `webkit-patch rebaseline-cl` will only download baselines for
tests that have unexpected results (either unexpectedly mismatching baselines or
unexpectedly missing baselines).

BUG=654604
==========

[qyearsley, 2016-11-01 21:17:00]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2016-11-01 21:17:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 22:36:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-01 22:38:46]

Description was changed from

==========
rebaseline-cl: Don't rebaseline expected-MISSING tests.

Before this CL: If there are tests with [ Missing ] expectations in
TestExpectations, then `webkit-patch rebaseline-cl` will download new baselines
for those tests. I think this behavior isn't quite what I think we want, since
any such tests are probably not directly related to the CL for which new
baselines are being downloaded.

After this CL: `webkit-patch rebaseline-cl` will only download baselines for
tests that have unexpected results (either unexpectedly mismatching baselines or
unexpectedly missing baselines).

BUG=654604
==========

to

==========
rebaseline-cl: Don't rebaseline expected-MISSING tests.

Before this CL: If there are tests with [ Missing ] expectations in
TestExpectations, then `webkit-patch rebaseline-cl` will download new baselines
for those tests. I think this behavior isn't quite what I think we want, since
any such tests are probably not directly related to the CL for which new
baselines are being downloaded.

After this CL: `webkit-patch rebaseline-cl` will only download baselines for
tests that have unexpected results (either unexpectedly mismatching baselines or
unexpectedly missing baselines).

BUG=654604

Committed: https://crrev.com/7526dc346a39764341c162734d57e48cea499ea2
Cr-Commit-Position: refs/heads/master@{#429138}
==========

[commit-bot: I haz the power, 2016-11-01 22:38:47]

Patchset 2 (id:??) landed as
https://crrev.com/7526dc346a39764341c162734d57e48cea499ea2
Cr-Commit-Position: refs/heads/master@{#429138}