Bug 264750 - WebKitFinder and TestPort disagree about layout_tests_dir
Summary: WebKitFinder and TestPort disagree about layout_tests_dir
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Sneddon [:gsnedders]
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-11-13 09:47 PST by Sam Sneddon [:gsnedders]
Modified: 2023-12-15 16:30 PST (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Sneddon [:gsnedders] 2023-11-13 09:47:40 PST
import unittest

from webkitpy.common.host_mock import MockHost
from webkitpy.common.webkit_finder import WebKitFinder
from webkitpy.port.test import TestPort


class TestPortTest(unittest.TestCase):

    def test_webkit_finder_base(self):
        host = MockHost()
        port = TestPort(host)
        fs = host.filesystem

        finder = WebKitFinder(fs)
        self.assertEqual(finder.layout_tests_dir(), port.layout_tests_dir())

E       AssertionError: '/mock-checkout/LayoutTests' != '/test.checkout/LayoutTests'
E       - /mock-checkout/LayoutTests
E       ?  ^^^^^
E       + /test.checkout/LayoutTests
E       ?  ^^^^^
Comment 1 Radar WebKit Bug Importer 2023-11-13 09:47:49 PST
<rdar://problem/118339735>
Comment 2 Sam Sneddon [:gsnedders] 2023-11-13 09:51:46 PST
Note if you replace port with host.port_factory.get() then this does work; so it's specifically about the interaction between TestPort (which tries to pretend it has its own directory paths) and the MockFileSystem.
Comment 3 Sam Sneddon [:gsnedders] 2023-11-14 04:57:50 PST
Excluding tests:

```
% rg '(mock-checkout)' Tools --glob '!*_unittest.py*' --glob '!*_integrationtest.py' --sort=path --no-heading
Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py:37:        self.checkout_root = "/mock-checkout"
Tools/Scripts/webkitpy/common/system/filesystem_mock.py:97:        return "/mock-checkout/Tools/Scripts/" + module_name.replace('.', '/') + ".py"
Tools/Scripts/webkitpy/port/darwin_testcase.py:60:            "MOCK popen: ['Tools/Scripts/run-safari', '--release', '--no-saved-state', '-NSOpen', 'test.html'], cwd=/mock-checkout\n",
Tools/Scripts/webkitpy/port/ios_testcase.py:41:        self.assertEqual(search_path[-1], '/mock-checkout/LayoutTests/platform/ios')
Tools/Scripts/webkitpy/port/ios_testcase.py:42:        self.assertEqual(search_path[-2], '/mock-checkout/LayoutTests/platform/ios-wk1')
Tools/Scripts/webkitpy/port/port_testcase.py:536:        self.assertEqual(port.path_to_test_expectations_file(), '/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations')
Tools/Scripts/webkitpy/port/port_testcase.py:540:        self.assertEqual(port.path_to_test_expectations_file(), '/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations')
Tools/Scripts/webkitpy/port/port_testcase.py:543:        port.host.filesystem.files['/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations'] = 'some content'
Tools/Scripts/webkitpy/port/port_testcase.py:545:        self.assertEqual(port.path_to_test_expectations_file(), '/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations')
Tools/Scripts/webkitpy/port/port_testcase.py:579:        host.filesystem.write_text_file('/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations',
Tools/Scripts/webkitpy/port/port_testcase.py:593:            "MOCK run_command: ['Tools/Scripts/build-dumprendertree', '--release'], cwd=/mock-checkout, env={'MOCK_ENVIRON_COPY': '1'}\n",
Tools/Scripts/webkitpy/port/port_testcase.py:602:            '''MOCK run_command: ['Tools/Scripts/build-dumprendertree', '--release'], cwd=/mock-checkout, env={'MOCK_ENVIRON_COPY': '1'}
Tools/Scripts/webkitpy/port/port_testcase.py:603:MOCK run_command: ['Tools/Scripts/build-webkittestrunner', '--release'], cwd=/mock-checkout, env={'MOCK_ENVIRON_COPY': '1'}
Tools/Scripts/webkitpy/port/port_testcase.py:613:            '''MOCK run_command: ['Tools/Scripts/build-dumprendertree', '--release'], cwd=/mock-checkout, env={'MOCK_ENVIRON_COPY': '1'}
Tools/Scripts/webkitpy/port/port_testcase.py:626:            '''MOCK run_command: ['Tools/Scripts/build-dumprendertree', '--release'], cwd=/mock-checkout, env={'MOCK_ENVIRON_COPY': '1'}
Tools/Scripts/webkitpy/port/port_testcase.py:689:        self.assertEqual(port._path_to_apache_config_file(), '/mock-checkout/LayoutTests/http/conf/httpd.conf')
Tools/Scripts/webkitpy/port/test.py:298:TEST_DIR = '/mock-checkout'
Tools/Scripts/webkitpy/port/watch_testcase.py:47:        self.assertEqual(search_path[-1], '/mock-checkout/LayoutTests/platform/wk2')
Tools/Scripts/webkitpy/port/watch_testcase.py:48:        self.assertEqual(search_path[-2], '/mock-checkout/LayoutTests/platform/watchos')
Tools/Scripts/webkitpy/tool/commands/queuestest.py:74:        checkout_dir = '/mock-checkout'

% rg '(test\.checkout)' Tools --glob '!*_unittest.py*' --glob '!*_integrationtest.py' --sort=path --no-heading
Tools/Scripts/webkitpy/port/test.py:296:LAYOUT_TEST_DIR = '/test.checkout/LayoutTests'
Tools/Scripts/webkitpy/port/test.py:297:PERF_TEST_DIR = '/test.checkout/PerformanceTests'
Tools/Scripts/webkitpy/port/test.py:477:        return '/test.checkout'
Tools/Scripts/webkitpy/port/test.py:653:        test_world_leaks_output = """TEST: file:///test.checkout/LayoutTests/failures/expected/leak.html
Tools/Scripts/webkitpy/port/test.py:654:ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/leak.html
Tools/Scripts/webkitpy/port/test.py:655:TEST: file:///test.checkout/LayoutTests/failures/unexpected/leak.html
Tools/Scripts/webkitpy/port/test.py:656:ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/flaky-leak.html
Tools/Scripts/webkitpy/port/test.py:657:TEST: file:///test.checkout/LayoutTests/failures/unexpected/flaky-leak.html
Tools/Scripts/webkitpy/port/test.py:658:ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/leak.html
Tools/Scripts/webkitpy/port/test.py:659:TEST: file:///test.checkout/LayoutTests/failures/unexpected/leak.html
Tools/Scripts/webkitpy/port/test.py:660:ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/leak-subframe.html
Tools/Scripts/webkitpy/port/test.py:661:TEST: file:///test.checkout/LayoutTests/failures/expected/leaky-reftest.html
Tools/Scripts/webkitpy/port/test.py:662:ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/leaky-reftest.html"""
Tools/Scripts/webkitpy/test/main.py:320:        "webkitscmpy.test.checkout_unittest",
```
Comment 4 Sam Sneddon [:gsnedders] 2023-11-14 05:06:55 PST
We have a comment in test.py:

```
# Here we use a non-standard location for the layout tests, to ensure that
# this works. The path contains a '.' in the name because we've seen bugs
# related to this before.

LAYOUT_TEST_DIR = '/test.checkout/LayoutTests'
PERF_TEST_DIR = '/test.checkout/PerformanceTests'
TEST_DIR = '/mock-checkout'
```

Which is an assumption that webkit_finder.py violates.

Ironically, the only place that uses WebKitFinder.layout_tests_dir is:

port/base.py
693:        return self._webkit_finder.layout_tests_dir()
Comment 5 Sam Sneddon [:gsnedders] 2023-11-14 08:23:44 PST
That said, port.base has a comment saying:

>     # FIXME: update callers to create a finder and call it instead of these next five routines (which should be protected).

So maybe it's simply not intended to allow ports to override the layout test directory any more?

That said, AFAICT no port ever in the tree ever did override it (I'd thought Chromium did, but it seemingly didn't), so maybe it's just needlessly generic?

If that's the case, then we can fix the difference by just dropping support for other paths.
Comment 6 Sam Sneddon [:gsnedders] 2023-11-15 10:42:54 PST
Okay, so while no _port_ overrides it, `port.base` does handle `--layout-tests-directory`, which _is_ used.

So we probably should push everything to go via the port, because otherwise we risk accidentally using different paths.
Comment 7 Sam Sneddon [:gsnedders] 2023-11-15 11:08:37 PST
Pull request: https://github.com/WebKit/WebKit/pull/20551
Comment 8 EWS 2023-11-17 03:45:46 PST
Committed 270880@main (cb302cd1f44f): <https://commits.webkit.org/270880@main>

Reviewed commits have been landed. Closing PR #20551 and removing active labels.