new-run-webkit-tests: use port_testcase in more port tests
Created attachment 74800 [details] Patch
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?
(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.
(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.
Created attachment 74804 [details] rename TestTest to TestPortTest
(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 ;)
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.