Bug 52762 – nrwt: remove fs refs from run_webkit_tests, rebaseline_chromium_webkit_tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt: remove fs refs from run_webkit_tests, rebaseline_chromium_webkit_tests

[Dirk Pranke]

Created attachment 79512 [details]
Patch

[Dirk Pranke]

Created attachment 79516 [details]
fix a few dangling references to test_filesystem in run_webkit_tests_unittest

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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

[Dirk Pranke]

Created attachment 79661 [details]
don't change MockFileSystem calls for now

[Mihai Parparita]

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

[Dirk Pranke]

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

[Dirk Pranke]

Created attachment 79668 [details]
update w/ review feedback from mihaip

[Dirk Pranke]

Committed r76321: <http://trac.webkit.org/changeset/76321>