nrwt: remove fs refs from run_webkit_tests, rebaseline_chromium_webkit_tests
Created attachment 79512 [details] Patch
Created attachment 79516 [details] fix a few dangling references to test_filesystem in run_webkit_tests_unittest
Comment on attachment 79516 [details] fix a few dangling references to test_filesystem in run_webkit_tests_unittest View in context: https://bugs.webkit.org/attachment.cgi?id=79516&action=review > Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py:91 > + filesystem = port.unit_test_filesystem() I'm confused. Why not just use MockFileSystem directly? Generally our live objects know nothing about mocks or unit tests.
(In reply to comment #3) > (From update of attachment 79516 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79516&action=review > > > Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py:91 > > + filesystem = port.unit_test_filesystem() > > I'm confused. Why not just use MockFileSystem directly? Generally our live objects know nothing about mocks or unit tests. In this particular test we probably could've used MockFileSystem directly, since we're not using the filesystem for anything in particular. In fact, it probably didn't even need to instantiate a filesystem at all. (I'll test that to make sure, and delete the ref if true). Otherwise, the unit_test_filesystem() gives me a consistent configuration to work with - a set of known filenames that should be present, a fixed set of test expectations, etc. This will be needed for other tests in this file coming up in a patch shortly. As to mixing live objects and mocks, assuming I understand you're concern, it's a valid point, but I'm not sure if there's a better way to get around the problem. I could import port.test.unit_test_filesystem, but it feels like bad style to mix package-level exports like port.get and reaching inside the package. I'm definitely open to other suggestions here.
Created attachment 79661 [details] don't change MockFileSystem calls for now
Comment on attachment 79661 [details] don't change MockFileSystem calls for now View in context: https://bugs.webkit.org/attachment.cgi?id=79661&action=review > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:82 > + last_unexpected_results = _gather_unexpected_results(port, options) Do you need to pass in the whole port, or is the filesystem enough? > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:410 > + with fs.mkdtemp() as tmpdir: Will need to import with from __future__.
(In reply to comment #6) > (From update of attachment 79661 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79661&action=review > > > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:82 > > + last_unexpected_results = _gather_unexpected_results(port, options) > > Do you need to pass in the whole port, or is the filesystem enough? > Good point. I can just pass in the filesystem. > > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:410 > > + with fs.mkdtemp() as tmpdir: > > Will need to import with from __future__. Good catch.
Created attachment 79668 [details] update w/ review feedback from mihaip
Committed r76321: <http://trac.webkit.org/changeset/76321>