Bug 50043 – new-run-webkit-tests: use port_testcase in more port tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

new-run-webkit-tests: use port_testcase in more port tests

[Dirk Pranke]

Created attachment 74800 [details]
Patch

[Tony Chang]

Comment on attachment 74800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74800&action=review

I guess we only run the test if the platform matches, right?  How much time does this add to the tests on cygwin and mac (where we're running 2 tests of tests)?

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:530
> +        if self.get_option('use_apache') == False:

Why is switching this if/else better?

Anyway, it should be "if not self...."

> WebKitTools/Scripts/webkitpy/layout_tests/port/test_unittest.py:31
> +import test

test seems like an unfortunate name for this file.

> WebKitTools/Scripts/webkitpy/layout_tests/port/test_unittest.py:35
> +class TestTest(port_testcase.PortTestCase):

TestTest seems like an even more unfortunate name.  Maybe TestPortTest or TestTestPort or TestForTestPort?

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 74800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74800&action=review
> 
> I guess we only run the test if the platform matches, right?  How much time does this add to the tests on cygwin and mac (where we're running 2 tests of tests)?
> 

We only run the test if the platform matches and the build is up to date (i.e., the binaries exist). This is to avoid trying to run Chromium tests in a non-Chromium checkout (or similar). The tests are slowish, and take a few seconds per file. Ideally they get turned on in "integration" mode when I am able to land that particular patch.
 
Dunno. Haven't actually actually run this on 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:530
> > +        if self.get_option('use_apache') == False:
> 
> Why is switching this if/else better?
> 

It changes the processing of 'use_apache' is not set (i.e., None). The old code would default to LigHTTPd, which is the wrong default for everyone but Chromium Win.

A better fix would be to actually pick up useful defaults; that fix falls in as part of bug 48095 .

> Anyway, it should be "if not self...."
> 

No, it shouldn't, it needs to be explicitly testing against False so that None is treated the same way as True. Arguably this could be written as if X is None or X is True, or we could use self.set_option_default(True).

> > WebKitTools/Scripts/webkitpy/layout_tests/port/test_unittest.py:31
> > +import test
> 
> test seems like an unfortunate name for this file.
>

Perhaps, but it has been named that way for quite some time. It is what you get if you say --platform test, so there it follows the convention all the the other files follow.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/test_unittest.py:35
> > +class TestTest(port_testcase.PortTestCase):
> 
> TestTest seems like an even more unfortunate name.  Maybe TestPortTest or TestTestPort or TestForTestPort?

Sure, I can change this to TestPortTest.

[Tony Chang]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 74800 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=74800&action=review
> > 
> It changes the processing of 'use_apache' is not set (i.e., None). The old code would default to LigHTTPd, which is the wrong default for everyone but Chromium Win.
> 
> A better fix would be to actually pick up useful defaults; that fix falls in as part of bug 48095 .

Let's fix that bug first.  Or more specifically, make the option value always True or False (can't we use store_result for this?)  Testing with == False seems fragile and non-obvious to anyone reading the code.

[Dirk Pranke]

Created attachment 74804 [details]
rename TestTest to TestPortTest

[Dirk Pranke]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 74800 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=74800&action=review
> > > 
> > It changes the processing of 'use_apache' is not set (i.e., None). The old code would default to LigHTTPd, which is the wrong default for everyone but Chromium Win.
> > 
> > A better fix would be to actually pick up useful defaults; that fix falls in as part of bug 48095 .
> 
> Let's fix that bug first.  Or more specifically, make the option value always True or False (can't we use store_result for this?)  Testing with == False seems fragile and non-obvious to anyone reading the code.

We can't use store_result, because we can't assume that the option was actually declared at all (that's part of the problem). 

I am fine w/ waiting on fixing 48095 if you are. I agree that testing specifically against False is somewhat fragile, especially if you are hard-wired to think that None and False are the same thing ;)

[Dirk Pranke]

Closing this ... we now use PortTestCase on all of the ports, and this patch is long-since obsolete.

There are still issues with the code and how we get default values, but those are tracked separately.