Summary: | derive ChromiumPort from WebKitPort in NRWT in order to support skipping tests if symbols are missing | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Raymond Toy <rtoy> | ||||||||||||
Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, dpranke, ojan, ossy, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 90112, 90134, 90371 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Raymond Toy
2012-06-21 16:21:25 PDT
Created attachment 148910 [details]
Patch
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. 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 ...
(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. Created attachment 149141 [details]
Patch
Comment on attachment 149141 [details]
Patch
That looks suspiciously easy... Please watch the bots carefully when you land this change.
Do all the port classes inherit from WebKitPort now? Can we merge WebKitPort and Port (as a separate patch obviously)? > Do all the port classes inherit from WebKitPort now? Yes. > Can we merge WebKitPort and Port (as a separate patch obviously)? Yes! :) (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. Created attachment 149397 [details]
make work with other libraries
Ray, can you check and make sure this revised patch works for you? 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). (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! (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. 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. Created attachment 149663 [details]
rename module_search_path to modules_to_search_for_symbols
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.
Committed r121363: <http://trac.webkit.org/changeset/121363> (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. (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. 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 :( (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 ... Re-opened since this is blocked by 90134 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 Created attachment 149999 [details]
Patch
(In reply to comment #25) > Created an attachment (id=149999) [details] > Patch Did you mean to mark this for review? 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 :).
Committed r121497: <http://trac.webkit.org/changeset/121497> (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 |