Bug 52771

Summary: nrwt: clean up almost all remaining port references
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, mihai, 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 Flags
Patch
none
update w/ review feedback from eseidel
none
Patch
none
update w/ review feedback from mihaip - fix mac_unittest none

Description Dirk Pranke 2011-01-19 17:17:24 PST
nrwt: clean up almost all remaining port references
Comment 1 Dirk Pranke 2011-01-19 17:18:40 PST
Created attachment 79527 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-01-20 03:02:23 PST
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?
Comment 3 Dirk Pranke 2011-01-20 15:58:49 PST
(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.
Comment 4 Dirk Pranke 2011-01-20 16:41:43 PST
Created attachment 79665 [details]
update w/ review feedback from eseidel
Comment 5 Dirk Pranke 2011-01-20 18:30:27 PST
Committed r76322: <http://trac.webkit.org/changeset/76322>
Comment 6 Csaba Osztrogon√°c 2011-01-21 00:25:40 PST
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?
Comment 7 Dirk Pranke 2011-01-21 14:03:26 PST
(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 ...
Comment 8 Dirk Pranke 2011-01-21 14:23:24 PST
Created attachment 79792 [details]
Patch
Comment 9 Tony Chang 2011-01-21 15:55:10 PST
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.
Comment 10 Mihai Parparita 2011-01-21 15:56:31 PST
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.
Comment 11 Dirk Pranke 2011-01-21 16:09:15 PST
(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.
Comment 12 Dirk Pranke 2011-01-21 16:11:17 PST
Created attachment 79807 [details]
update w/ review feedback from mihaip - fix mac_unittest
Comment 13 Dirk Pranke 2011-01-21 17:30:07 PST
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>
Comment 14 Dirk Pranke 2011-01-21 17:30:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Review Bot 2011-01-21 18:01:04 PST
http://trac.webkit.org/changeset/76413 might have broken Qt Linux Release
The following tests are not passing:
fast/text/justify-nbsp.html