Bug 58288 – webkitpy: fix mock_filesystem abspath to handle relative paths

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

[Dirk Pranke]

webkitpy: fix mock_filesystem abspath to handle relative paths

[Dirk Pranke]

Created attachment 89122 [details]
Patch

[Dirk Pranke]

This patch is needed to fix the way NRWT handles relative paths in --results-directory on a mock filesystem (right now MockFileSystem.abspath() is totally broken for relative paths).

[Eric Seidel (no email)]

Comment on attachment 89122 [details]
Patch

Just pick a fake path for getcwd().  Like /tmp/fakepath.  We shouldn't depend on cwd in the unit tests.

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 89122 [details])
> Just pick a fake path for getcwd().  Like /tmp/fakepath.  We shouldn't depend on cwd in the unit tests.

I agree that we shouldn't depend on the cwd in the unit tests.  If you look at the tests in bug 58272 (where this ends up getting set), we mock out os.getcwd() there, which is the only place (so far) where this functionality is needed.

I'm not exactly sure what you mean by pick a fake path, but if you mean we should just use "/tmp/fakepath" instead of calling os.getcwd() inside this file, then I'm a bit reluctant to do that because it might limit what can be tested where.

[Eric Seidel (no email)]

We pick a fake path for Scm.checkout_root for similar reasons.  I agree it would be ugly to put /tmp/fakepath (or similar) in one place.  So maybe we should have some shared fake cwd variable.

I mean, the patch is OK.  But dependencies on os.getcwd() are going to come back and bite us.  We have slowly been removing our os.chdir() calls (which were previously needed to keep the unit tests passing).  I worry that this one could be giving us another source of such trouble.

[Dirk Pranke]

(In reply to comment #5)
> We pick a fake path for Scm.checkout_root for similar reasons.  I agree it would be ugly to put /tmp/fakepath (or similar) in one place.  So maybe we should have some shared fake cwd variable.
> 
> I mean, the patch is OK.  But dependencies on os.getcwd() are going to come back and bite us.  We have slowly been removing our os.chdir() calls (which were previously needed to keep the unit tests passing).  I worry that this one could be giving us another source of such trouble.

I agree with what you are saying. I thought about adding filesystem.getcwd() and filesystem.setcwd() but they felt wrong because it's a property of the running process, not the filesystem. I couldn't think of a better place to add it, but maybe filesystem is better than nothing?

[Dirk Pranke]

Created attachment 89237 [details]
add chdir/getcwd so we can test and mock relative paths properly

[Eric Seidel (no email)]

Comment on attachment 89237 [details]
add chdir/getcwd so we can test and mock relative paths properly

View in context: https://bugs.webkit.org/attachment.cgi?id=89237&action=review

OK.  This seems useful.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:70
> +        # FIXME: we probably need some consistent way of mocking out getcwd.

Except don't you plan to use it here?

[Dirk Pranke]

Created attachment 89244 [details]
fix bug in abspath, clean up handling of directories and normalized paths

[Dirk Pranke]

(In reply to comment #8)
> (From update of attachment 89237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89237&action=review
> 
> OK.  This seems useful.
> 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:70
> > +        # FIXME: we probably need some consistent way of mocking out getcwd.
> 
> Except don't you plan to use it here?

Yeah, this is the downside of (a) breaking a single patch into three patches and (b) not having unit tests for filesystem_mock. We probably need to write the latter at some point since more and more things are depending on it and the logic is getting bigger and bigger.

[Eric Seidel (no email)]

Comment on attachment 89244 [details]
fix bug in abspath, clean up handling of directories and normalized paths

View in context: https://bugs.webkit.org/attachment.cgi?id=89244&action=review

OK.  Seems reasonable.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:52
> +        self.dirs = {}

I"m unclear on exactly what this does.  I assume this is dirs for the cwd?  Or is this all dirs for the mock file system? I'm unclear why its part of thsi change, but I also doubt it matters. :)

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 89244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89244&action=review
> 
> OK.  Seems reasonable.
> 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:52
> > +        self.dirs = {}
> 
> I"m unclear on exactly what this does.  I assume this is dirs for the cwd?  Or is this all dirs for the mock file system? I'm unclear why its part of thsi change, but I also doubt it matters. :)

The previous logic for figuring out whether or not something was a directory relied on prefix matches of the file names in self.files. If you set the cwd to a value that didn't match any of the existing files, it would have failed subsequent isdir() checks. In addition, maybe_make_dir() wasn't implemented, which meant that you couldn't create a directory and then cd to it.

All of these things were needed in order for test that actually uses to pass.

[Dirk Pranke]

Committed r83631: <http://trac.webkit.org/changeset/83631>