Bug 52762 - nrwt: remove fs refs from run_webkit_tests, rebaseline_chromium_webkit_tests
Summary: nrwt: remove fs refs from run_webkit_tests, rebaseline_chromium_webkit_tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-19 16:10 PST by Dirk Pranke
Modified: 2011-01-20 18:27 PST (History)
3 users (show)

See Also:


Attachments
Patch (12.33 KB, patch)
2011-01-19 16:11 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix a few dangling references to test_filesystem in run_webkit_tests_unittest (12.09 KB, patch)
2011-01-19 16:20 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
don't change MockFileSystem calls for now (10.54 KB, patch)
2011-01-20 15:47 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback from mihaip (10.62 KB, patch)
2011-01-20 17:07 PST, Dirk Pranke
mihai: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-01-19 16:10:15 PST
nrwt: remove fs refs from run_webkit_tests, rebaseline_chromium_webkit_tests
Comment 1 Dirk Pranke 2011-01-19 16:11:09 PST
Created attachment 79512 [details]
Patch
Comment 2 Dirk Pranke 2011-01-19 16:20:47 PST
Created attachment 79516 [details]
fix a few dangling references to test_filesystem in run_webkit_tests_unittest
Comment 3 Eric Seidel (no email) 2011-01-20 03:04:59 PST
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.
Comment 4 Dirk Pranke 2011-01-20 15:13:45 PST
(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.
Comment 5 Dirk Pranke 2011-01-20 15:47:29 PST
Created attachment 79661 [details]
don't change MockFileSystem calls for now
Comment 6 Mihai Parparita 2011-01-20 16:41:50 PST
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__.
Comment 7 Dirk Pranke 2011-01-20 17:00:36 PST
(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.
Comment 8 Dirk Pranke 2011-01-20 17:07:21 PST
Created attachment 79668 [details]
update w/ review feedback from mihaip
Comment 9 Dirk Pranke 2011-01-20 18:27:09 PST
Committed r76321: <http://trac.webkit.org/changeset/76321>