Add PRESUBMIT checks for not required testharness expected files.

Add PRESUBMIT checks for not required testharness expected files.

This will prevent *-expected.txt files to be added when they are not
needed. It will prevent the test runner to do a file comparison when it
could simply check the output of the test.

BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181313

[mlamouri (slow - plz ping), 2014-06-11 22:28:02]

Peter and Dirk, could you have a look at this CL please? I also have a CL to
remove the files that are positive (per check-testharness-expected-pass):
https://codereview.chromium.org/332583002

[mlamouri (slow - plz ping), 2014-06-11 22:28:47]

On 2014/06/11 22:28:02, Mounir Lamouri wrote:
> Peter and Dirk, could you have a look at this CL please? I also have a CL to
> remove the files that are positive (per check-testharness-expected-pass):
> https://codereview.chromium.org/332583002

Hmm, I might have meant https://codereview.chromium.org/328243002/ ;)

[Dirk Pranke, 2014-06-13 22:27:07]

The basic idea seems fine.

I would not create a separate package for this; just put testharness_results in
layout_tests/models or something like that.

https://codereview.chromium.org/332583002/diff/20001/Tools/Scripts/webkitpy/l...
File
Tools/Scripts/webkitpy/layout_tests/testharness/testharness_results_checker.py
(right):

https://codereview.chromium.org/332583002/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/testharness/testharness_results_checker.py:36:
def is_testharness_output_passing(content_text):
Can you rewrite this as:

def is_testharness_output_passing(content_text):
    if not is_testharness_output(content_text):
        return False
    lines = [line.strip() for line in content_text.strip().splitlines()]
    return not any(line.startswith('FAIL') or line.startswith('TIMEOUT') or
line.startswith('NOTRUN') or line.startswith('Harness Error')
                           for line in lines)

?

Alternatively, you could refactor both routines to operate on an array of
stripped lines so that you don't have to split and strip them twice.

The basic idea is not to duplicate the overall format checking twice.

[mlamouri (slow - plz ping), 2014-06-15 12:12:41]

I moved the files and removed the tests in port/test.py given that the test
runner simply calls testharness_results and wasn't testing more than that.

https://codereview.chromium.org/332583002/diff/20001/Tools/Scripts/webkitpy/l...
File
Tools/Scripts/webkitpy/layout_tests/testharness/testharness_results_checker.py
(right):

https://codereview.chromium.org/332583002/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/testharness/testharness_results_checker.py:36:
def is_testharness_output_passing(content_text):
On 2014/06/13 22:27:07, Dirk Pranke wrote:
> Can you rewrite this as:
> 
> def is_testharness_output_passing(content_text):
>     if not is_testharness_output(content_text):
>         return False
>     lines = [line.strip() for line in content_text.strip().splitlines()]
>     return not any(line.startswith('FAIL') or line.startswith('TIMEOUT') or
> line.startswith('NOTRUN') or line.startswith('Harness Error')
>                            for line in lines)
> 
> ?
> 
> Alternatively, you could refactor both routines to operate on an array of
> stripped lines so that you don't have to split and strip them twice.
> 
> The basic idea is not to duplicate the overall format checking twice.

The only reason why I'm checking the format here is because we currently are
very conservative in the way we check test output and we expect the test to
contain no unexpected line. I'm not sure  if that's the right way to do it
honestly. I would tend to think that it's too conservative and we should allow
any kind of output (basically, this rule mean that a test would have to cleanup
any HTML elements with a text content).

Your proposal doesn't take that into account and would fail some test cases.

So, the only way to prevent checking the output twice would have to have one
internal method that checks the format and the passing at the same time and both
public methods would use that internal one. However, I don't think that's the
way to go forward. I think we could simply let unexpected output being allowed,
in which case I could have a passing check looking roughly as you suggested but
I would prefer that to be in a separate CL.

WDYT?

[Dirk Pranke, 2014-06-16 21:14:42]

On 2014/06/15 12:12:41, Mounir Lamouri wrote:
> I moved the files and removed the tests in port/test.py given that the test
> runner simply calls testharness_results and wasn't testing more than that.
> 
>
https://codereview.chromium.org/332583002/diff/20001/Tools/Scripts/webkitpy/l...
> File
> Tools/Scripts/webkitpy/layout_tests/testharness/testharness_results_checker.py
> (right):
> 
>
https://codereview.chromium.org/332583002/diff/20001/Tools/Scripts/webkitpy/l...
>
Tools/Scripts/webkitpy/layout_tests/testharness/testharness_results_checker.py:36:
> def is_testharness_output_passing(content_text):
> On 2014/06/13 22:27:07, Dirk Pranke wrote:
> > Can you rewrite this as:
> > 
> > def is_testharness_output_passing(content_text):
> >     if not is_testharness_output(content_text):
> >         return False
> >     lines = [line.strip() for line in content_text.strip().splitlines()]
> >     return not any(line.startswith('FAIL') or line.startswith('TIMEOUT') or
> > line.startswith('NOTRUN') or line.startswith('Harness Error')
> >                            for line in lines)
> > 
> > ?
> > 
> > Alternatively, you could refactor both routines to operate on an array of
> > stripped lines so that you don't have to split and strip them twice.
> > 
> > The basic idea is not to duplicate the overall format checking twice.
> 
> The only reason why I'm checking the format here is because we currently are
> very conservative in the way we check test output and we expect the test to
> contain no unexpected line. I'm not sure  if that's the right way to do it
> honestly ...
> 
> Your proposal doesn't take that into account and would fail some test cases.

True, my proposal forgot to check for 'PASS'; that's easily fixed.

I see little advantage to allowing arbitrary output; doing so allows people to
write sloppy tests, and makes it harder to explain what the format is expected
to be. 

My understanding of how people think of testharness-based tests is that they run
a series of assertions, just like most other testing frameworks. I wish to keep
that conceptual model, and the more arbitrary stuff we allow, the weirder that
becomes.

In other words, I strongly prefer that we keep the conservative approach for
now. 

> So, the only way to prevent checking the output twice would have to have one
> internal method that checks the format and the passing at the same time and
both
> public methods would use that internal one. However, I don't think that's the
> way to go forward.

Yes, I think you should do this.

> I think we could simply let unexpected output being allowed,
> in which case I could have a passing check looking roughly as you suggested
but
> I would prefer that to be in a separate CL.

As noted above, I do not think we should let unexpected output be allowed.

[mlamouri (slow - plz ping), 2014-08-31 19:43:17]

On 2014/06/16 21:14:42, Dirk Pranke wrote:
> True, my proposal forgot to check for 'PASS'; that's easily fixed.
> 
> I see little advantage to allowing arbitrary output; doing so allows people to
> write sloppy tests, and makes it harder to explain what the format is expected
> to be. 
> 
> My understanding of how people think of testharness-based tests is that they
run
> a series of assertions, just like most other testing frameworks. I wish to
keep
> that conceptual model, and the more arbitrary stuff we allow, the weirder that
> becomes.
> 
> In other words, I strongly prefer that we keep the conservative approach for
> now. 

Sorry for taking so long to get back to this CL.

I was not very clear in my previous CL. What I was trying to point is that I
must check for the format while checking for the result because we want to fail
the test if the output contains something unexpected. By definition, that means
that the result checker knows about the expected output. If we want to reduce
the check as "all lines are PASS or CONSOLE", we would need to have the
testharness header and footer removed before. We could test that "all lines are
PASS, CONSOLE, HEADER or FOOTER" but that's basically what I'm doing with as
special block for expected errors (in case of we want to have different failure
behaviour when the test failed compared to when the test has unexpected output
(could be two different error code).

In other words, as long as we want the result check to be format depend (ie.
rejects all unexpected output), we can't prevent checking for the format while
checking for the result. What we could do though is to have one method that
verifies the format and the result and result two boolean or a tri-state boolean
(Pass, Fail, FormatError). However, even though it would refactor this, I think
it would not be a good idea because it would make the format checker and the
result checker too much linked. Some subtleties of one might not be very
interesting for the other: the format checker want to make sure the format is
right while the result checker want to make sure the output is passing so having
5 HEADER lines would be wrong for the former but fine for the latter. Finally,
merging the two would make any change in the result checker harder.

Sorry again for taking so long to reply. Could you PTAL based on that?

[Dirk Pranke, 2014-09-02 23:48:17]

I guess what I was thinking is that you'd have a single routine that returned
two booleans (or a tri-state enum). You could perhaps make that routine private
and expose two public functions if need be.

It's true that the logic of the two routines would be pretty strongly coupled,
but that's true regardless (as implied by your precondition on one of the two
routines).

However, I don't feel that strongly about this.

lgtm w/ the comments addressed.

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py (right):

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py:18: """
I would just use the first line of the docstring, and delete the "Returns: ..."
which is redundant. In webkitpy we don't explicitly document arguments and
return types.

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py:43: True if
content_text is a passing testharness output. False otherwise.
Same thing, I would delete lines 41-30 (but keep the note).

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py:50: # The
check is very conservative and reject any unexpected content in the output.
nit: grammar: "rejects", not "reject".

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py:66: # Those
are expected failing outpu.
Nit: typo: "output", not "outpu"

[mlamouri (slow - plz ping), 2014-09-03 10:53:46]

Comments applied. I would be okay to have a private method that would be used by
the two methods but this CL stayed pending for quite a while and I feel that
none of us feel very strong about doing that. If one of us or someone else want
to do a change in that direction later, I would be up for it. Anyway, this code
is tested so any refactoring should be fairly safe.

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py (right):

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py:18: """
On 2014/09/02 23:48:17, Dirk Pranke wrote:
> I would just use the first line of the docstring, and delete the "Returns:
..."
> which is redundant. In webkitpy we don't explicitly document arguments and
> return types.

Done.

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py:43: True if
content_text is a passing testharness output. False otherwise.
On 2014/09/02 23:48:17, Dirk Pranke wrote:
> Same thing, I would delete lines 41-30 (but keep the note).

Done. I assumed that you meant 41-43.

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py:50: # The
check is very conservative and reject any unexpected content in the output.
On 2014/09/02 23:48:17, Dirk Pranke wrote:
> nit: grammar: "rejects", not "reject".

Done.

https://codereview.chromium.org/332583002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py:66: # Those
are expected failing outpu.
On 2014/09/02 23:48:17, Dirk Pranke wrote:
> Nit: typo: "output", not "outpu"

Done.

[mlamouri (slow - plz ping), 2014-09-03 11:01:10]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-03 11:01:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/332583002/60001

[commit-bot: I haz the power, 2014-09-03 11:54:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/22016)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/25362)

[commit-bot: I haz the power, 2014-09-03 12:04:32]

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

[commit-bot: I haz the power, 2014-09-03 12:04:33]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[mlamouri (slow - plz ping), 2014-09-03 12:29:59]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-03 12:30:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/332583002/80001

[commit-bot: I haz the power, 2014-09-03 13:34:08]

Committed patchset #5 (id:80001) as 181313