[test] Presumbit check against missing tests in status files

[test] Presumbit check against missing tests in status files

Our test infrastructure ignores missing tests which are listed in
status files. Sometimes, tests are removed and status file lines
are not updated. This patch adds a presubmit check for status
files addressing JavaScript tests to not reference missing tests.
It also cleans up existing violations.

R=machenbach

Review-Url: https://codereview.chromium.org/2610353002
Cr-Commit-Position: refs/heads/master@{#42106}
Committed: https://chromium.googlesource.com/v8/v8/+/32c1a7933c705cf1d179ecf835efa66ca54e00e5

[Dan Ehrenberg, 2017-01-05 17:16:45]

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

[commit-bot: I haz the power, 2017-01-05 17:16:48]

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, 2017-01-05 17:40:57]

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

[commit-bot: I haz the power, 2017-01-05 17:40:57]

Dry run: This issue passed the CQ dry run.

[Michael Achenbach, 2017-01-06 09:13:38]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2017-01-06 09:13:39]

lgtm

https://codereview.chromium.org/2610353002/diff/1/tools/testrunner/local/stat...
File tools/testrunner/local/statusfile.py (right):

https://codereview.chromium.org/2610353002/diff/1/tools/testrunner/local/stat...
tools/testrunner/local/statusfile.py:258: if 'test262' in path and '*' not in
rule:
Could you make this more generic for all the suites with js tests? E.g.
debugger, webkit, etc? Maybe with a toplevel dict with the configs. Or at least
add a TODO.

[Dan Ehrenberg, 2017-01-06 09:48:31]

https://codereview.chromium.org/2610353002/diff/1/tools/testrunner/local/stat...
File tools/testrunner/local/statusfile.py (right):

https://codereview.chromium.org/2610353002/diff/1/tools/testrunner/local/stat...
tools/testrunner/local/statusfile.py:258: if 'test262' in path and '*' not in
rule:
On 2017/01/06 09:13:39, Michael Achenbach (OOO) wrote:
> Could you make this more generic for all the suites with js tests? E.g.
> debugger, webkit, etc? Maybe with a toplevel dict with the configs. Or at
least
> add a TODO.

Done.

[Dan Ehrenberg, 2017-01-06 09:49:03]

Description was changed from

==========
[test] Presumbit check against missing tests in status files

Our test infrastructure ignores missing tests which are listed in
status files. Sometimes, tests are removed and status file lines
are not updated. This patch adds a presubmit check for mjsunit and
test262 status files to not reference missing tests. It also cleans
up existing violations.

R=machenbach
==========

to

==========
[test] Presumbit check against missing tests in status files

Our test infrastructure ignores missing tests which are listed in
status files. Sometimes, tests are removed and status file lines
are not updated. This patch adds a presubmit check for status
files addressing JavaScript tests to not reference missing tests.
It also cleans up existing violations.

R=machenbach
==========

[Dan Ehrenberg, 2017-01-06 09:49:10]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2017-01-06 09:49:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
machenbach@chromium.org
Link to the patchset: https://codereview.chromium.org/2610353002/#ps20001
(title: "Generalize to all JS tests")

[commit-bot: I haz the power, 2017-01-06 09:49:17]

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, 2017-01-06 10:13:44]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483696150494760,
"parent_rev": "a6402fd91667fbb955e5f990c70e09c32770ecea", "commit_rev":
"32c1a7933c705cf1d179ecf835efa66ca54e00e5"}

[commit-bot: I haz the power, 2017-01-06 10:13:50]

Description was changed from

==========
[test] Presumbit check against missing tests in status files

Our test infrastructure ignores missing tests which are listed in
status files. Sometimes, tests are removed and status file lines
are not updated. This patch adds a presubmit check for status
files addressing JavaScript tests to not reference missing tests.
It also cleans up existing violations.

R=machenbach
==========

to

==========
[test] Presumbit check against missing tests in status files

Our test infrastructure ignores missing tests which are listed in
status files. Sometimes, tests are removed and status file lines
are not updated. This patch adds a presubmit check for status
files addressing JavaScript tests to not reference missing tests.
It also cleans up existing violations.

R=machenbach

Review-Url: https://codereview.chromium.org/2610353002
Cr-Commit-Position: refs/heads/master@{#42106}
Committed:
https://chromium.googlesource.com/v8/v8/+/32c1a7933c705cf1d179ecf835efa66ca54...
==========

[commit-bot: I haz the power, 2017-01-06 10:13:50]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/32c1a7933c705cf1d179ecf835efa66ca54...

[Michael Achenbach, 2017-01-06 14:34:40]

Thanks for making it generic. I wonder if it will work at all though. There is
code in presubmit that only triggers the check when certain files are changed in
a CL. In this case I think only status files are included. Now we also need to
include all test files (that were deleted) and DEPS changes to cover test262.

See:
https://cs.chromium.org/chromium/src/v8/PRESUBMIT.py?l=82
And:
https://cs.chromium.org/chromium/src/v8/tools/presubmit.py?l=469

In this case the logic is different. Before, we would check the file that has
changed. Now we need to check the status file if a particular test file got
deleted. Maybe pragmatically we should just activate the status file check
always. It was like that until recently. At least if the whole check doesn't
take much longer than 2-3 seconds.

[Dan Ehrenberg, 2017-01-06 14:49:00]

On 2017/01/06 14:34:40, Michael Achenbach (OOO) wrote:
> Thanks for making it generic. I wonder if it will work at all though. There is
> code in presubmit that only triggers the check when certain files are changed
in
> a CL. In this case I think only status files are included. Now we also need to
> include all test files (that were deleted) and DEPS changes to cover test262.
> 
> See:
> https://cs.chromium.org/chromium/src/v8/PRESUBMIT.py?l=82
> And:
> https://cs.chromium.org/chromium/src/v8/tools/presubmit.py?l=469
> 
> In this case the logic is different. Before, we would check the file that has
> changed. Now we need to check the status file if a particular test file got
> deleted. Maybe pragmatically we should just activate the status file check
> always. It was like that until recently. At least if the whole check doesn't
> take much longer than 2-3 seconds.

Oh, my mistake, I verified this by modifying status files :) The idea of running
always on status files seems reasonable to me (since there aren't many status
files), though the current logic should be fine for things like test262 rolls.

[Michael Achenbach, 2017-01-09 08:48:26]

> Oh, my mistake, I verified this by modifying status files :) The idea of
running
> always on status files seems reasonable to me (since there aren't many status
> files), though the current logic should be fine for things like test262 rolls.

The current logic will be fine as long as every roll requires a status file
change as well. Probably true most of the time...