[webkit-reviews] review granted: [Bug 38063] new-run-webkit-tests complains about missing pixel results instead of plopping down new expectations : [Attachment 99121] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 38063: new-run-webkit-tests complains about missing pixel results instead
of plopping down new expectations
https://bugs.webkit.org/show_bug.cgi?id=38063

Attachment 99121: Patch
https://bugs.webkit.org/attachment.cgi?id=99121&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
Generally looks good ... R+'ing it although there are some nits and at least
one change that looks correct but unnecessary that I have a question about.

View in context: https://bugs.webkit.org/attachment.cgi?id=99121&action=review

> Tools/Scripts/webkitpy/common/net/layouttestresults.py:164
> +	   return [result for result in self._test_results if
result.has_failure_matching_types(*failure_types)]

Why make this change (and the one in test_results.py)? It seems unnecessary.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:309
> +	       dest="save_missing_results", default=True,

Nit: might this be better as dest="new_test_results"? That's more in keeping
with how we normally name options.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:798
> +	   failures = self._get_failures(result_summary, include_crashes=False,
include_missing=False)

Hm. I'm not sure about this change, given that we can have flaky MISSING
results if a text-only test crashes or fails prior to calling dumpAsText(). I
agree that there's no point in retrying a truly missing test, and maybe the
flaky case is rare enough that this is the right thing to do.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:936
> +    def _get_failures(self, result_summary, include_crashes=True,
include_missing=True):

Nit: I'd probably leave off the default values, since it's an internal function
and it looks like they're not being used anyway.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:131
> +	   if self._options.save_missing_results:

Change to options.new_test_results as per above?

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:145
> +    def _add_missing_baselines(self, test_result, driver_output):

Nit: maybe add comments or docstrings indicating which options trigger
_add_missing_baselines and _overwrite_baselines? I know you can figure this out
from looking at the callers, but it might be helpful to list them here as well.


> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:150
> +	       # than for text tests?

If I remember correctly, with old-run-webkit-tests, all new test results go in
the generic directories alongside the test, and you need
--add-platform-exceptions to tell it to put the new results in the platform dir
instead.

Also, I suppose theoretically at least some render tree dumps could be generic
(enough for more platforms, anyway).

Regardless, there's no way to distinguish a render tree dump from a
non-rendertree dump without parsing the text output, and I'd prefer to not have
to start doing that. 

Maybe we can modify DRT to output custom headers when dumping the layer tree?

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:160
>	   # DumpRenderTree may not output utf-8 text (e.g. webarchives).

As an aside, it turns out webarchives are just XML files and I think they're
always UTF-8 as a result. Should probably update this comment at some point ...


> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:190
> +	       _log.debug('Writing new baseline result "%s"' % output_path)

I'm tempted to say that these should be _log.info() calls ... that would
address Ojan's comments about the "silent" updates, and probably is generally
more useful.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:193
> +	       _log.debug('Resetting baseline result "%s"' % output_path)

Same as above.