Bug 52771 – nrwt: clean up almost all remaining port references

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: clean up almost all remaining port references

[Dirk Pranke]

Created attachment 79527 [details]
Patch

[Eric Seidel (no email)]

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?

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 79665 [details]
update w/ review feedback from eseidel

[Dirk Pranke]

Committed r76322: <http://trac.webkit.org/changeset/76322>

[Csaba Osztrogonác]

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?

[Dirk Pranke]

(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 ...

[Dirk Pranke]

Created attachment 79792 [details]
Patch

[Tony Chang]

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.

[Mihai Parparita]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 79807 [details]
update w/ review feedback from mihaip - fix mac_unittest

[Dirk Pranke]

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>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/76413 might have broken Qt Linux Release
The following tests are not passing:
fast/text/justify-nbsp.html