[webkit-reviews] review denied: [Bug 57987] new-run-webkit-tests: implement support for audio files : [Attachment 88554] more updates, testing, add note about missing LF after content-length

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 57987: new-run-webkit-tests: implement support for audio files
https://bugs.webkit.org/show_bug.cgi?id=57987

Attachment 88554: more updates, testing, add note about missing LF after
content-length
https://bugs.webkit.org/attachment.cgi?id=88554&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88554&action=review

Close, but seems like we could use one more round.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:95
> +				   
self._port.expected_checksum(self._filename),
> +				   
audio=self._port.expected_audio(self._filename))

Nit: It seems inconsistent that only audio is named.  Maybe name the other
keyword params or remove audio= ?

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:244
> +	       if actual_audio and expected_audio is None:

if there's no actual_audio, but expected_audio is None, shouldn't that still be
a missing audio error?	E.g., _compare_text doesn't do this check.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:258
> +	   if not output:
> +	       return output

Why do we need this?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:45
> +(PASS, FAIL, TEXT, IMAGE, IMAGE_PLUS_TEXT, AUDIO, TIMEOUT,
> + CRASH, SKIP, WONTFIX, SLOW, REBASELINE, MISSING, FLAKY, NOW, NONE) =
range(16)

Nit: The wrapping here seems really arbitrary.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:69
> +	   elif isinstance(failure, test_failures.FailureAudioMismatch):
> +	       writer.write_audio_files(driver_output.audio,
expected_driver_output.audio)
> +	   elif isinstance(failure, test_failures.FailureMissingAudio):
> +	       writer.write_audio_files(driver_output.audio,
expected_audio=None)

Can we just use an or here?  I think expected_driver_output.audio would be None
in the missing case.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:215
> +	   if False and actual_audio:

Did you mean to remove the False here?	How do the tests pass?

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:420
> +	   if block.content_type == 'audio/wav':
> +	       audio = block.decoded_content
> +	   else:
> +	       output = block.decoded_content

I would probably push more of this logic into ContentBlock.  E.g.,
audio = block.get_audio_content()
output = block.get_text_content()

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:426
> +	   if block.content and block.content_type == 'image/png':
> +	       image = block.decoded_content
> +	       actual_image_hash = block.content_hash

This as well, so you wouldn't need to pre-declare these variables.