[test+presubmit] Remove duplicate test status file entries

[test+presubmit] Remove duplicate test status file entries

And add a presubmit check to guard against future duplicates.

R=machenbach@chromium.org

Committed: https://crrev.com/f7a0ecb0caeb2e0368a6b6e2abd03b447edc6dbe
Cr-Commit-Position: refs/heads/master@{#32380}

[Jakob Kummerow, 2015-11-27 12:53:45]

PTAL. I took the liberty to make the check you've recently added
multi-error-proof.

[Michael Achenbach, 2015-11-27 14:24:20]

lgtm with some suggestions/comments. Thanks a lot for working on this!

https://codereview.chromium.org/1477403002/diff/20001/test/test262/test262.st...
File test/test262/test262.status (left):

https://codereview.chromium.org/1477403002/diff/20001/test/test262/test262.st...
test/test262/test262.status:67:
'language/statements/const/syntax/const-invalid-assignment-next-expression-for':
[PASS, FAIL, TIMEOUT],
Maybe preserve this comment by moving it to the duplicate below. Maybe CC
littledan for test262 status changes...

https://codereview.chromium.org/1477403002/diff/20001/tools/presubmit.py
File tools/presubmit.py (right):

https://codereview.chromium.org/1477403002/diff/20001/tools/presubmit.py#newc...
tools/presubmit.py:462: if not statusfile.PresubmitCheck(status_file_path):
suggestion: Maybe rewrite this with:

success &= Check1()
success &= Check2()

https://codereview.chromium.org/1477403002/diff/20001/tools/testrunner/local/...
File tools/testrunner/local/statusfile.py (right):

https://codereview.chromium.org/1477403002/diff/20001/tools/testrunner/local/...
tools/testrunner/local/statusfile.py:159: def _Assert(check, message, status):
nit: naming (optional). Since it's nested, the _ is not necessary and functions
(at least nested) usually start with small letter. Unfortunately "assert" is
already taken ;) Maybe call it check?

https://codereview.chromium.org/1477403002/diff/20001/tools/testrunner/local/...
tools/testrunner/local/statusfile.py:164: status = {"success": True}
As talked offline. You could move this a few lines up and then skip passing it
as additional parameter.

https://codereview.chromium.org/1477403002/diff/20001/tools/testrunner/local/...
tools/testrunner/local/statusfile.py:167: _Assert(type(section) == list,
"Section must be a list", status)
Just for the record. Also as discussed offline. If any of these checks really
fails, the subsequent ones might run into less readable errors.

E.g. a section that is an int will trigger the first _Assert and then cause an
exception on the len call below...

[Jakob Kummerow, 2015-11-27 14:43:26]

Thanks for the review, landing.

https://codereview.chromium.org/1477403002/diff/20001/test/test262/test262.st...
File test/test262/test262.status (left):

https://codereview.chromium.org/1477403002/diff/20001/test/test262/test262.st...
test/test262/test262.status:67:
'language/statements/const/syntax/const-invalid-assignment-next-expression-for':
[PASS, FAIL, TIMEOUT],
On 2015/11/27 14:24:20, Michael Achenbach wrote:
> Maybe preserve this comment by moving it to the duplicate below. Maybe CC
> littledan for test262 status changes...

Done.

https://codereview.chromium.org/1477403002/diff/20001/tools/presubmit.py
File tools/presubmit.py (right):

https://codereview.chromium.org/1477403002/diff/20001/tools/presubmit.py#newc...
tools/presubmit.py:462: if not statusfile.PresubmitCheck(status_file_path):
On 2015/11/27 14:24:20, Michael Achenbach wrote:
> suggestion: Maybe rewrite this with:
> 
> success &= Check1()
> success &= Check2()

Done.

https://codereview.chromium.org/1477403002/diff/20001/tools/testrunner/local/...
File tools/testrunner/local/statusfile.py (right):

https://codereview.chromium.org/1477403002/diff/20001/tools/testrunner/local/...
tools/testrunner/local/statusfile.py:159: def _Assert(check, message, status):
On 2015/11/27 14:24:20, Michael Achenbach wrote:
> nit: naming (optional). Since it's nested, the _ is not necessary and
functions
> (at least nested) usually start with small letter. Unfortunately "assert" is
> already taken ;) Maybe call it check?

Compromise: _assert (to emphasize that it's a customized variant of the default
"assert").

https://codereview.chromium.org/1477403002/diff/20001/tools/testrunner/local/...
tools/testrunner/local/statusfile.py:164: status = {"success": True}
On 2015/11/27 14:24:20, Michael Achenbach wrote:
> As talked offline. You could move this a few lines up and then skip passing it
> as additional parameter.

Done.

https://codereview.chromium.org/1477403002/diff/20001/tools/testrunner/local/...
tools/testrunner/local/statusfile.py:167: _Assert(type(section) == list,
"Section must be a list", status)
On 2015/11/27 14:24:20, Michael Achenbach wrote:
> Just for the record. Also as discussed offline. If any of these checks really
> fails, the subsequent ones might run into less readable errors.
> 
> E.g. a section that is an int will trigger the first _Assert and then cause an
> exception on the len call below...

Keeping as is, reasoning: it's rather unlikely that someone would run into
these, so we shouldn't overengineer it.

[Jakob Kummerow, 2015-11-27 14:43:39]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2015-11-27 14:43:39]

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/1477403002/#ps40001
(title: "address comments")

[commit-bot: I haz the power, 2015-11-27 14:43:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477403002/40001

[commit-bot: I haz the power, 2015-11-27 15:04:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-27 15:04:53]

Description was changed from

==========
[test+presubmit] Remove duplicate test status file entries

And add a presubmit check to guard against future duplicates.

R=machenbach@chromium.org
==========

to

==========
[test+presubmit] Remove duplicate test status file entries

And add a presubmit check to guard against future duplicates.

R=machenbach@chromium.org

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

[commit-bot: I haz the power, 2015-11-27 15:04:53]

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

[Michael Achenbach, 2015-11-27 15:05:21]

lgtm