Bug 89706 – derive ChromiumPort from WebKitPort in NRWT in order to support skipping tests if symbols are missing

• Component: New Bugs
• Status: RESOLVED FIXED

[Raymond Toy]

Add support for missing symbols to skip tests

[Raymond Toy]

Created attachment 148910 [details]
Patch

[Raymond Toy]

Here is proposed patch to allow the missing symbols to skip tests.  I added a version of _missing_symbol_to_skipped_tests with the two additional symbols, ff_mp3_decoder and ff_aac_decoder, to test if these symbols are not defined, then the corresponding webaudio codec tests are skipped if the mp3 or aac decoder is not available.  This seems to work.

One difference I do see is that the timeouts are huge (35000 and 175000).

I only tested this by running the webaudio tests since I'm familiar with those.  I didn't try anything else.

[Dirk Pranke]

Comment on attachment 148910 [details]
Patch

Thanks for working on this Ray. There's definitely a couple of things that I need to review and fix from this patch, so I'll take it from here ...

[Raymond Toy]

(In reply to comment #3)
> (From update of attachment 148910 [details])
> Thanks for working on this Ray. There's definitely a couple of things that I need to review and fix from this patch, so I'll take it from here ...

Thanks for your help in this!  Looking forward to having this feature.

[Dirk Pranke]

Created attachment 149141 [details]
Patch

[Adam Barth]

Comment on attachment 149141 [details]
Patch

That looks suspiciously easy...  Please watch the bots carefully when you land this change.

[Ojan Vafai]

Do all the port classes inherit from WebKitPort now? Can we merge WebKitPort and Port (as a separate patch obviously)?

[Adam Barth]

> Do all the port classes inherit from WebKitPort now?

Yes.

> Can we merge WebKitPort and Port (as a separate patch obviously)?

Yes!  :)

[Dirk Pranke]

(In reply to comment #8)
> > Do all the port classes inherit from WebKitPort now?
> 
> Yes.
> 

Well, almost. The test classes don't yet, but that's not terribly important.

> > Can we merge WebKitPort and Port (as a separate patch obviously)?
> 
> Yes!  :)

Indeed, that's the plan.

[Dirk Pranke]

Created attachment 149397 [details]
make work with other libraries

[Dirk Pranke]

Ray, can you check and make sure this revised patch works for you?

[Raymond Toy]

Comment on attachment 149397 [details]
make work with other libraries

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

Still running tests, but these two items showed up.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:91
> +    def _module_search_path(self):

Typo?  I think it should be module_search_paths (with an s).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py:115
> +    def _module_search_path(self):

Typo?  I think it should be module_search_paths (with an s).

[Raymond Toy]

(In reply to comment #11)
> Ray, can you check and make sure this revised patch works for you?

Tested this on mac, with the two typos fixed.  Everything runs as expected.

Thanks for the update!

[Raymond Toy]

(In reply to comment #13)
> (In reply to comment #11)
> > Ray, can you check and make sure this revised patch works for you?
> 
> Tested this on mac, with the two typos fixed.  Everything runs as expected.
> 
> Thanks for the update!

Tested this on linux.  Tests are skipped as expected if ffmpeg doesn't include the necessary codecs.

On windows, the tests are always skipped, as expected.

[Dirk Pranke]

Comment on attachment 149397 [details]
make work with other libraries

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:91
>> +    def _module_search_path(self):
> 
> Typo?  I think it should be module_search_paths (with an s).

Actually, module_search_path isn't a very good name. Will fix.

[Dirk Pranke]

Created attachment 149663 [details]
rename module_search_path to modules_to_search_for_symbols

[Dirk Pranke]

Comment on attachment 149663 [details]
rename module_search_path to modules_to_search_for_symbols

okay, I think this patch is good to go. I'll land it tomorrow morning when I can be around to see if anything explodes.

[Dirk Pranke]

Committed r121363: <http://trac.webkit.org/changeset/121363>

[Csaba Osztrogonác]

(In reply to comment #18)
> Committed r121363: <http://trac.webkit.org/changeset/121363>

It broke NRWT on Qt:
Traceback (most recent call last):
  File "/ramdisk/qt-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 456, in <module>
    sys.exit(main())
  File "/ramdisk/qt-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 451, in main
    return run(port, options, args)
  File "/ramdisk/qt-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 118, in run
    manager.parse_expectations()
  File "/ramdisk/qt-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 376, in parse_expectations
    self._expectations = test_expectations.TestExpectations(self._port, self._test_files)
  File "/ramdisk/qt-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 743, in __init__
    self._add_skipped_tests(port.skipped_layout_tests(tests).union(set(port.get_option('ignore_tests', []))))
  File "/ramdisk/qt-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 372, in skipped_layout_tests
    tests_to_skip.update(self._skipped_tests_for_unsupported_features(test_list))
  File "/ramdisk/qt-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 342, in _skipped_tests_for_unsupported_features
    symbols_string = self._symbols_string()
  File "/ramdisk/qt-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 279, in _symbols_string
    symbols += self._executive.run_command([self.nm_command(), path_o_module], error_handler=Executive.ignore_error)
NameError: global name 'path_o_module' is not defined
Failed to execute Tools/Scripts/new-run-webkit-tests at ./Tools/Scripts/run-webkit-tests line 124.

[Dirk Pranke]

(In reply to comment #19)
> (In reply to comment #18)
> > Committed r121363: <http://trac.webkit.org/changeset/121363>
> 
> It broke NRWT on Qt:

bah! fixing ... one sec.

[Dirk Pranke]

landed a patch to fix the typo:

http://trac.webkit.org/changeset/121367

I will now try to figure out why this both passed my unit tests *and* me running it interactively :(

[Dirk Pranke]

(In reply to comment #21)
> landed a patch to fix the typo:
> 
> http://trac.webkit.org/changeset/121367
> 
> I will now try to figure out why this both passed my unit tests *and* me running it interactively :(

Sigh, the typo occurs in the line invoking the actual 'nm' call, which is mocked out in the unit test, and when I ran it interactively I was only running a subset of the tests, none of which might've been skipped by this :(.

I have been meaning to get 'pylint' configured for webkitpy, that would've caught this ...

[WebKit Review Bot]

Re-opened since this is blocked by 90134

[Csaba Osztrogonác]

The following changes were rolled out to unbreak debug bots:
http://trac.webkit.org/changeset/121363
http://trac.webkit.org/changeset/121367 - typo fix after r121363
http://trac.webkit.org/changeset/121384 - fix after r121363
http://trac.webkit.org/changeset/121390  - typo fix after r121390

Rollout landed in http://trac.webkit.org/changeset/121406

[Dirk Pranke]

Created attachment 149999 [details]
Patch

[Ojan Vafai]

(In reply to comment #25)
> Created an attachment (id=149999) [details]
> Patch

Did you mean to mark this for review?

[Dirk Pranke]

Comment on attachment 149999 [details]
Patch

No, I wasn't going to make someone suffer throught re-reviewing all of the test-related changes, but if you wanted to, that would be great. Given the number of typos/reverts/breakages, it's probably a good idea :).

[Dirk Pranke]

Committed r121497: <http://trac.webkit.org/changeset/121497>

[Csaba Osztrogonác]

(In reply to comment #28)
> Committed r121497: <http://trac.webkit.org/changeset/121497>

It switched off and broke many unittests, I filed a new bug report for it:
https://bugs.webkit.org/show_bug.cgi?id=90371