Use Port.skips_test in copy-existing-baselines command.

Use Port.skips_test in copy-existing-baselines command.

After this CL, I expect that when rebaselining (with rebaseline-cl or other rebaseline commands), new baselines shouldn't be added for non-smoke-tests on android.

BUG=655196
Committed: https://crrev.com/f2543789d3b0638e3afed6efaf0a9e8e244a3c42
Cr-Commit-Position: refs/heads/master@{#428540}

[qyearsley, 2016-10-27 21:37:53]

Description was changed from

==========
Use Port.skips_test in copy-existing-baselines command.

BUG=655196
==========

to

==========
Use Port.skips_test in copy-existing-baselines command.

After this CL, I expect that when rebaselining (with rebaseline-cl or other
rebaseline commands), new baselines shouldn't be added for non-smoke-tests on
android.

BUG=655196
==========

[qyearsley, 2016-10-27 21:37:53]

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

[qyearsley, 2016-10-27 21:41:01]

Note: I'm not sure whether a test for this belongs in rebaseline_unittest, or
whether it would make more sense to put it in port/base_unittest.

Also, I think that skips_test probably doesn't need to take two TestExpectations
objects, and it should just need to take "full_expectations" (the copy that
includes overriding test expectations files); so I think this could be done in a
follow-up?

[Dirk Pranke, 2016-10-27 22:27:34]

lgtm.

On 2016/10/27 21:41:01, qyearsley wrote:
> Note: I'm not sure whether a test for this belongs in rebaseline_unittest, or
> whether it would make more sense to put it in port/base_unittest.

I'd put the tests for skips_tests() in port/base_unittest.py

> 
> Also, I think that skips_test probably doesn't need to take two
TestExpectations
> objects, and it should just need to take "full_expectations" (the copy that
> includes overriding test expectations files); so I think this could be done in
a
> follow-up?

Okay, I stared at this some more to try and remember why things are this way,
and I think I know now.

The problem is that you need to distinguish between a test that is being skipped
temporarily due to 
a bug (in which case we should rebaseline it) and a test that is intentionally
skipped because we 
never want to run it. The latter class of things are specified in NeverFixTests.
By computing the 
two sets of expectations with and without including NeverFixTests, you're able
to figure out 
which case you have.

Assuming I'm right here, obviously we could make that clearer in the code.

[qyearsley, 2016-10-28 00:13:02]

On 2016/10/27 at 22:27:34, dpranke wrote:
> lgtm.
> 
> On 2016/10/27 21:41:01, qyearsley wrote:
> > Note: I'm not sure whether a test for this belongs in rebaseline_unittest,
or
> > whether it would make more sense to put it in port/base_unittest.
> 
> I'd put the tests for skips_tests() in port/base_unittest.py
> 
> > 
> > Also, I think that skips_test probably doesn't need to take two
TestExpectations
> > objects, and it should just need to take "full_expectations" (the copy that
> > includes overriding test expectations files); so I think this could be done
in a
> > follow-up?
> 
> Okay, I stared at this some more to try and remember why things are this way,
and I think I know now.
> 
> The problem is that you need to distinguish between a test that is being
skipped temporarily due to 
> a bug (in which case we should rebaseline it) and a test that is intentionally
skipped because we 
> never want to run it. The latter class of things are specified in
NeverFixTests. By computing the 
> two sets of expectations with and without including NeverFixTests, you're able
to figure out 
> which case you have.
> 
> Assuming I'm right here, obviously we could make that clearer in the code.

Ah! Makes sense.

In a follow-up I'd like to add clarifying comments and tests for skips_test.

[qyearsley, 2016-10-28 00:13:05]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2016-10-28 00:13:27]

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-28 01:39:03]

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

[commit-bot: I haz the power, 2016-10-28 01:39:04]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qyearsley, 2016-10-28 22:00:02]

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

[commit-bot: I haz the power, 2016-10-28 22:00:32]

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

[qyearsley, 2016-10-28 22:02:30]

On 2016/10/28 at 01:39:04, commit-bot wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

So, the current unit test for copy-existing-baselines tests that the behavior is
that baseline shouldn't be copied to a platform's directory if the test is
skipped in the generic test expectations file.

Since the purpose of this CL is just to make it so that baselines aren't copied
to android for non-smoke-tests (http://crbug.com/655196), I think I'd like to
commit this with a minor tweak and then follow-up with another CL. Does this
look OK?

[Dirk Pranke, 2016-10-28 22:09:00]

On 2016/10/28 22:02:30, qyearsley wrote:
> On 2016/10/28 at 01:39:04, commit-bot wrote:
> > Try jobs failed on following builders:
> >   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> So, the current unit test for copy-existing-baselines tests that the behavior
is
> that baseline shouldn't be copied to a platform's directory if the test is
> skipped in the generic test expectations file.
> 
> Since the purpose of this CL is just to make it so that baselines aren't
copied
> to android for non-smoke-tests (http://crbug.com/655196), I think I'd like to
> commit this with a minor tweak and then follow-up with another CL. Does this
> look OK?

That's fine, I think.

[commit-bot: I haz the power, 2016-10-28 23:11:25]

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

[commit-bot: I haz the power, 2016-10-28 23:11:26]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-10-28 23:13:59]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-10-28 23:13:59]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2455233002/#ps20001
(title: "For copying existing baselines, continue skipping if test is skipped in
generic expectations.")

[commit-bot: I haz the power, 2016-10-28 23:14:18]

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-28 23:23:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-28 23:26:53]

Description was changed from

==========
Use Port.skips_test in copy-existing-baselines command.

After this CL, I expect that when rebaselining (with rebaseline-cl or other
rebaseline commands), new baselines shouldn't be added for non-smoke-tests on
android.

BUG=655196
==========

to

==========
Use Port.skips_test in copy-existing-baselines command.

After this CL, I expect that when rebaselining (with rebaseline-cl or other
rebaseline commands), new baselines shouldn't be added for non-smoke-tests on
android.

BUG=655196

Committed: https://crrev.com/f2543789d3b0638e3afed6efaf0a9e8e244a3c42
Cr-Commit-Position: refs/heads/master@{#428540}
==========

[commit-bot: I haz the power, 2016-10-28 23:26:54]

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

[chenwilliam, 2016-10-31 22:49:29]

Just an FYI.

https://codereview.chromium.org/2455233002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2455233002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:933: runs
smoke tests only, or because the
I think this patch truncated the comment on accident (I just noticed it while
working on this file).

[qyearsley, 2016-10-31 23:05:21]

https://codereview.chromium.org/2455233002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2455233002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:933: runs
smoke tests only, or because the
On 2016/10/31 at 22:49:29, chenwilliam wrote:
> I think this patch truncated the comment on accident (I just noticed it while
working on this file).

Good catch, thanks! I'll fix this in a follow-up.