webkitpy: fix mock_filesystem abspath to handle relative paths
Created attachment 89122 [details] Patch
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).
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.
(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.
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.
(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?
Created attachment 89237 [details] add chdir/getcwd so we can test and mock relative paths properly
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?
Created attachment 89244 [details] fix bug in abspath, clean up handling of directories and normalized paths
(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.
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. :)
(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.
Committed r83631: <http://trac.webkit.org/changeset/83631>