Summary: | nrwt: clean up almost all remaining port references | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||||||
Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, eric, mihaip, ojan, ossy, rgabor, tony, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Other | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 52772 | ||||||||||||
Attachments: |
|
Description
Dirk Pranke
2011-01-19 17:17:24 PST
Created attachment 79527 [details]
Patch
Comment on attachment 79527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79527&action=review Looks sane. Are you sure all the import with_statement removals are safe? > Tools/ChangeLog:8 > + Need a short description and bug URL (OOPS!) Missing bug url/title. > Tools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py:48 > + self._port = None Shouldn't be needed. > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:67 > if not port.check_image_diff(): > + # The port hasn't been built - don't run the tests. > return I'm confused why the tests depend on having a build here. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:399 > + environment['DUMPRENDERTREE_TEMP'] = str(self._driver_tempdir) Curious why this is needed? (In reply to comment #2) > (From update of attachment 79527 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79527&action=review > > Looks sane. Are you sure all the import with_statement removals are safe? > I double-checked them all. The only one that wasn't was test_runner.py, which was fixed last night in a separate patch (this patch was stale). > > Tools/ChangeLog:8 > > + Need a short description and bug URL (OOPS!) > > Missing bug url/title. > Added. > > Tools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py:48 > > + self._port = None > > Shouldn't be needed. > It isn't needed, but it'll keep me from accidentally referencing it in a different test down the road. > > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:67 > > if not port.check_image_diff(): > > + # The port hasn't been built - don't run the tests. > > return > > I'm confused why the tests depend on having a build here. > This test is an integration test that launches ImageDiff and checks the result ... if there is no build, it can't execute properly, so we skip it. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:399 > > + environment['DUMPRENDERTREE_TEMP'] = str(self._driver_tempdir) > > Curious why this is needed? I don't actually know, but I think it might have to do with tests for LocalStorage or something. Created attachment 79665 [details]
update w/ review feedback from eseidel
Committed r76322: <http://trac.webkit.org/changeset/76322> Reopen, because it broke NRWT on Qt: ( http://webkit.sed.hu/buildbot/waterfall?show=x86-32%20Linux%20Qt%20Release%20new-run-webkit-tests ) expected result: => Results: 17414/22954 tests passed (75.9%) => Tests to be fixed (5540): 1 test timed out ( 0.0%) 88 no expected results found ( 1.6%) 10 text diff mismatch ( 0.2%) 5441 skipped (98.2%) actual results: => Results: 17269/22954 tests passed (75.2%) => Tests to be fixed (5685): 5 DumpRenderTree crashes ( 0.1%) 22 tests timed out ( 0.4%) 1533 no expected results found (27.0%) 3648 text diff mismatch (64.2%) 477 skipped ( 8.4%) It seems NRWT ignores platform/qt/Skipped file. Gabor, could you check it please? (In reply to comment #6) > It seems NRWT ignores platform/qt/Skipped file. > Gabor, could you check it please? I can confirm that I broke this. Patch coming up shortly ... Created attachment 79792 [details]
Patch
Comment on attachment 79792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79792&action=review > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:102 > +if __name__ == '__main__': > + unittest.main() I guess the imports only work if you happen to run this from the right place? Anyway, seems harmless. View in context: https://bugs.webkit.org/attachment.cgi?id=79792&action=review > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:305 > + tests_to_skip.extend(self._tests_from_skipped_file_contents(skipped_file_contents)) Does mac_unittest still pass with this change? The mac port class extends WebKitPort, and mac_unittest.py calls _tests_from_skipped_file. (In reply to comment #10) > View in context: https://bugs.webkit.org/attachment.cgi?id=79792&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:305 > > + tests_to_skip.extend(self._tests_from_skipped_file_contents(skipped_file_contents)) > > Does mac_unittest still pass with this change? The mac port class extends WebKitPort, and mac_unittest.py calls _tests_from_skipped_file. Good catch. I would've sworn I ran the tests before uploading the patch, but I guess I didn't. Created attachment 79807 [details]
update w/ review feedback from mihaip - fix mac_unittest
Comment on attachment 79807 [details] update w/ review feedback from mihaip - fix mac_unittest Clearing flags on attachment: 79807 Committed r76413: <http://trac.webkit.org/changeset/76413> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/76413 might have broken Qt Linux Release The following tests are not passing: fast/text/justify-nbsp.html |