Bug 89706

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 BugsAssignee: 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 Flags
Patch
none
Patch
none
make work with other libraries
none
rename module_search_path to modules_to_search_for_symbols
none
Patch ojan: review+

Description Raymond Toy 2012-06-21 16:21:25 PDT
Add support for missing symbols to skip tests
Comment 1 Raymond Toy 2012-06-21 16:21:54 PDT
Created attachment 148910 [details]
Patch
Comment 2 Raymond Toy 2012-06-21 16:29:06 PDT
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 3 Dirk Pranke 2012-06-21 16:58:53 PDT
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 ...
Comment 4 Raymond Toy 2012-06-22 08:50:40 PDT
(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.
Comment 5 Dirk Pranke 2012-06-22 16:45:03 PDT
Created attachment 149141 [details]
Patch
Comment 6 Adam Barth 2012-06-22 16:50:58 PDT
Comment on attachment 149141 [details]
Patch

That looks suspiciously easy...  Please watch the bots carefully when you land this change.
Comment 7 Ojan Vafai 2012-06-23 07:40:17 PDT
Do all the port classes inherit from WebKitPort now? Can we merge WebKitPort and Port (as a separate patch obviously)?
Comment 8 Adam Barth 2012-06-23 11:27:38 PDT
> Do all the port classes inherit from WebKitPort now?

Yes.

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

Yes!  :)
Comment 9 Dirk Pranke 2012-06-23 13:23:54 PDT
(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.
Comment 10 Dirk Pranke 2012-06-25 16:55:55 PDT
Created attachment 149397 [details]
make work with other libraries
Comment 11 Dirk Pranke 2012-06-25 16:57:45 PDT
Ray, can you check and make sure this revised patch works for you?
Comment 12 Raymond Toy 2012-06-25 21:24:02 PDT
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).
Comment 13 Raymond Toy 2012-06-25 22:21:50 PDT
(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!
Comment 14 Raymond Toy 2012-06-26 09:32:27 PDT
(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 15 Dirk Pranke 2012-06-26 18:46:43 PDT
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.
Comment 16 Dirk Pranke 2012-06-26 18:49:10 PDT
Created attachment 149663 [details]
rename module_search_path to modules_to_search_for_symbols
Comment 17 Dirk Pranke 2012-06-26 18:50:26 PDT
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.
Comment 18 Dirk Pranke 2012-06-27 13:53:12 PDT
Committed r121363: <http://trac.webkit.org/changeset/121363>
Comment 19 Csaba Osztrogonác 2012-06-27 14:07:04 PDT
(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.
Comment 20 Dirk Pranke 2012-06-27 14:07:49 PDT
(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.
Comment 21 Dirk Pranke 2012-06-27 14:10:25 PDT
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 :(
Comment 22 Dirk Pranke 2012-06-27 14:18:40 PDT
(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 ...
Comment 23 WebKit Review Bot 2012-06-27 22:12:34 PDT
Re-opened since this is blocked by 90134
Comment 24 Csaba Osztrogonác 2012-06-27 22:25:31 PDT
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
Comment 25 Dirk Pranke 2012-06-28 13:41:10 PDT
Created attachment 149999 [details]
Patch
Comment 26 Ojan Vafai 2012-06-28 14:23:54 PDT
(In reply to comment #25)
> Created an attachment (id=149999) [details]
> Patch

Did you mean to mark this for review?
Comment 27 Dirk Pranke 2012-06-28 15:22:40 PDT
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 :).
Comment 28 Dirk Pranke 2012-06-28 18:14:44 PDT
Committed r121497: <http://trac.webkit.org/changeset/121497>
Comment 29 Csaba Osztrogonác 2012-07-02 03:33:49 PDT
(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