Bug 57987 – new-run-webkit-tests: implement support for audio files

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

new-run-webkit-tests: implement support for audio files

[Dirk Pranke]

Created attachment 88524 [details]
Patch

[Dirk Pranke]

Okay, I think this patch should do the trick. Chris, I haven't applied your audio patch locally yet to test it, so I'll do that now.

Ito-san, all of your refactoring paid off - adding the new failure type was dirt simple :).

[Dirk Pranke]

Assuming we're relatively happy with how this is working in NRWT, adding support to ORWT should be pretty easy.

[Tony Chang]

Comment on attachment 88524 [details]
Patch

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

r- because no tests and until it's clear whether we have .txt and .wav or just .wav.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:85
> +            for suffix in ['.txt', '.checksum', '.png', '.wav']:

Nit: [] to ()

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:241
> +            if expected_audio == '':

'' or None?  It looks like this will be None if the file is not found.  Maybe just change this to 'if not expected_audio:'

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:44
> +(PASS, FAIL, TEXT, IMAGE, IMAGE_PLUS_TEXT, AUDIO, AUDIO_PLUS_TEXT, TIMEOUT,

I thought these tests only generated .wav files (i.e., no text file)?

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:447
>              elif line.startswith('Content-Type:'):
> -                pass
> +                content_type = ' '.split(line)[1]
> +            elif line.startswith('Content-Transfer-Encoding:'):

Should Content-Type: and Content-Transfer-Encoding: be consts like Content-Length:?

[Ojan Vafai]

Can you give some context for this? What would a test that uses audio look like?

[Tony Chang]

(In reply to comment #5)
> Can you give some context for this? What would a test that uses audio look like?

https://bugs.webkit.org/show_bug.cgi?id=57969

[Dirk Pranke]

Created attachment 88548 [details]
Patch

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 88524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88524&action=review
> 
> r- because no tests and until it's clear whether we have .txt and .wav or just .wav.
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:85
> > +            for suffix in ['.txt', '.checksum', '.png', '.wav']:
> 
> Nit: [] to ()
> 

Will change in the third patch.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:241
> > +            if expected_audio == '':
> 
> '' or None?  It looks like this will be None if the file is not found.  Maybe just change this to 'if not expected_audio:'
>

Yup, changed to None.
 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:44
> > +(PASS, FAIL, TEXT, IMAGE, IMAGE_PLUS_TEXT, AUDIO, AUDIO_PLUS_TEXT, TIMEOUT,
> 
> I thought these tests only generated .wav files (i.e., no text file)?
>

Yup, changed.
 
> > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:447
> >              elif line.startswith('Content-Type:'):
> > -                pass
> > +                content_type = ' '.split(line)[1]
> > +            elif line.startswith('Content-Transfer-Encoding:'):
> 
> Should Content-Type: and Content-Transfer-Encoding: be consts like Content-Length:?

Yup, changed.

Note the second patch adds a bunch of tests and also shuffles the logic a bit so that tests will pass when we have an audio file but no text file (previously we always assumed we'd get a text file and would report MISSING if it didn't exist).

[Dirk Pranke]

Created attachment 88553 [details]
more testing, review feedback from tony, update mock_drt

[Dirk Pranke]

Created attachment 88554 [details]
more updates, testing, add note about missing LF after content-length

[Tony Chang]

Comment on attachment 88554 [details]
more updates, testing, add note about missing LF after content-length

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.

[Dirk Pranke]

Comment on attachment 88554 [details]
more updates, testing, add note about missing LF after content-length

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:95
>> +                                 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= ?

Will fix by adding keyword args to the others; I didn't want to splice in the audio param and potentially break any other caller that wasn't using the names, and the constructors takes like twelve arguments (unfortunately).

>> 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.

The routines in the Port class and DriverOutput are not consistent over how None and '' should be interpreted. I was reluctant to try and fix this as part of this patch in order to avoid changing too many things at once. I think maybe I'll try to clean up the existing code first in a separate patch and then update this accordingly and hopefully that will clarify some of these issues.

That said, if there's no expected audio, and no actual audio, then the text simply doesn't use audio and while the audio is "missing", it's not an error.

compare_text didn't have this logic, because before this change, there was *always* audio and hence if it was missing, it was always an error. We get around this by the "if" block above on line 222. A different way to interpret this would be to push the if check into _compare_text and remove the if on line 222, if possible. I'll see what I can do to try and clean that up.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:258
>> +            return output
> 
> Why do we need this?

if output == None, the replace() crashes. This is related to the whole None vs. '' confusion, above.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:45
>> + CRASH, SKIP, WONTFIX, SLOW, REBASELINE, MISSING, FLAKY, NOW, NONE) = range(16)
> 
> Nit: The wrapping here seems really arbitrary.

I'll see if I can make it slightly less arbitrary.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:69
>> +            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.

Possibly. I'll try it and see if there's some catch that isn't immediately obvious.

>> 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?

Yes, I meant to remove the False.

>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:420
>> +            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()

I'm not sure I see the advantage here. ContentBlock is at the moment just a struct. A single block can't have multiple values at once, so both of those routines would have to be if statements that either return the actual content or None, which I don't think makes things much clearer. Maybe I'm not seeing what you have in mind?

>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:426
>> +            actual_image_hash = block.content_hash
> 
> This as well, so you wouldn't need to pre-declare these variables.

You lost me here. How do I get away with not pre-declaring the variables? I need them to be defined so that they can be returned on line 439.

[Tony Chang]

Comment on attachment 88554 [details]
more updates, testing, add note about missing LF after content-length

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

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:258
>>> +            return output
>> 
>> Why do we need this?
> 
> if output == None, the replace() crashes. This is related to the whole None vs. '' confusion, above.

I mean, why are we calling _get_normalized_output_text/_compare_text on None?  In the case of audio data, we don't need to call _compare_text, do we?

>>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:420
>>> +            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()
> 
> I'm not sure I see the advantage here. ContentBlock is at the moment just a struct. A single block can't have multiple values at once, so both of those routines would have to be if statements that either return the actual content or None, which I don't think makes things much clearer. Maybe I'm not seeing what you have in mind?

Right, it would just move the if statement into the class.  It would save having to write audio = None and output = None at the top of this method.  I don't feel strongly about this change.

[Dirk Pranke]

(In reply to comment #13)
> (From update of attachment 88554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88554&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:258
> >>> +            return output
> >> 
> >> Why do we need this?
> > 
> > if output == None, the replace() crashes. This is related to the whole None vs. '' confusion, above.
> 
> I mean, why are we calling _get_normalized_output_text/_compare_text on None?  In the case of audio data, we don't need to call _compare_text, do we?

It's possible this is a left over result of some intermediate steps in getting this all working. I'll try to get this all cleaned up as part of the cleanup of the None/'' issue.

> 
> >>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:420
> >>> +            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()
> > 
> > I'm not sure I see the advantage here. ContentBlock is at the moment just a struct. A single block can't have multiple values at once, so both of those routines would have to be if statements that either return the actual content or None, which I don't think makes things much clearer. Maybe I'm not seeing what you have in mind?
> 
> Right, it would just move the if statement into the class.  It would save having to write audio = None and output = None at the top of this method.  I don't feel strongly about this change.

I think this wil muddy the purpose of _read_block() and ContentBlock, so I'd prefer to leave it as-is.

[Dirk Pranke]

Created attachment 88756 [details]
split into two patches

[Dirk Pranke]

Okay, I've moved most of the generic cleanup into bug 58101, but I left the refactoring of the reading of blocks of output from DRT in this patch. Theoretically I could split this into yet another patch, but this patch is pretty readable as is and most of the changes make sense in the context of the audio change.

[Tony Chang]

Comment on attachment 88756 [details]
split into two patches

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

> Tools/Scripts/webkitpy/layout_tests/port/base.py:882
> +    def __init__(self, text=None, image=None, image_hash=None, audio=None,
> +                 crash=False, test_time=0, timeout=False, error=''):

Nit: Making all the params named parameters makes things noisier to me and it's inconsistent with what we do for other functions.  I liked the old way better.

[Dirk Pranke]

Created attachment 88857 [details]
Patch

[Dirk Pranke]

Committed r83330: <http://trac.webkit.org/changeset/83330>