Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added methods to access real files #171

Merged
merged 5 commits into from
Apr 14, 2017

Conversation

mrbean-bremen
Copy link
Member

@mrbean-bremen mrbean-bremen self-assigned this Apr 1, 2017
@mrbean-bremen
Copy link
Member Author

@jmcgeheeiv - please review!

Copy link
Contributor

@jmcgeheeiv jmcgeheeiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the comments I added in-line, I am concerned that this will not work in the context of fake_filesystem_unittest. This is based on my experience with CopyRealFile().

I originally put CopyRealFile() in class FakeFilesystem. Even the unit tests (of the same style you are using) worked. However, when it was in class FakeFilesystem, I had no control over whether it was using the real or fake file methods because fake_filesystem_unittest patches all the file method.

After discovering this, In PR #157, commit a890807, I moved it to fake_filesystem_unittest.TestCase.copyRealFile().

Please add a usage example to example.py and example_test.py and see what happens.

@@ -886,7 +897,7 @@ def CollapsePath(self, path):
continue
if component == '..':
if collapsed_path_components and (
collapsed_path_components[-1] != '..'):
collapsed_path_components[-1] != '..'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent?

CHANGES.md Outdated
@@ -4,6 +4,7 @@ The release versions are PyPi releases.
## Version 3.2 (as yet unreleased)

#### New Features
* Added possibility to add lazily read real files to fake filesystem
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps:
Added new methods that make real files and directories appear within the fake file system:

  • fake_filesystem.FakeFilesystem.AddRealFile()
  • fake_filesystem.FakeFilesystem.AddRealDirectory()
  • fake_filesystem.FakeFilesystem.AddRealPaths()

File contents are read from the real file system only when needed.

self.byte_contents = self._EncodeContents(contents, encoding)
self.st_size = len(self.byte_contents) if self.byte_contents else 0
self._byte_contents = self._EncodeContents(contents, encoding)
self.st_size = len(self._byte_contents) if self._byte_contents else 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if self.byte_contents is not None

@@ -823,7 +834,7 @@ def GetOpenFile(self, file_des):
if not isinstance(file_des, int):
raise TypeError('an integer is required')
if (file_des >= len(self.open_files) or
self.open_files[file_des] is None):
self.open_files[file_des] is None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent?

@@ -918,7 +929,7 @@ def NormalizeCase(self, path):
for component in path_components:
dir_name, current_dir = self._DirectoryContent(current_dir, component)
if current_dir is None or (
current_dir.byte_contents is None and current_dir.st_size == 0):
current_dir._byte_contents is None and current_dir.st_size == 0):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent?

@@ -1003,7 +1014,7 @@ def SplitDrive(self, path):
# UNC path handling is here since Python 2.7.8, back-ported from Python 3
if sys.version_info >= (2, 7, 8):
if (path[0:2] == self.path_separator * 2) and (
path[2:3] != self.path_separator):
path[2:3] != self.path_separator):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't notice these changes - PyCharm has sometimes its own ideas about hanging indents, obviously I did an auto-format accidentally. I will revert this.

@mrbean-bremen
Copy link
Member Author

I added an example as requested. I had to use REAL_OPEN to compare the content with the real file, but you won't need this in a real test, so I think the API can be used in unit tests without problems.

@mrbean-bremen
Copy link
Member Author

Also - as CopyRealFile has at least one additional feature (copy to another location), I think it has its own value, but I don't think we need another addition to TestCase - the methods in fake_filesystem should be sufficient.

@mrbean-bremen
Copy link
Member Author

@dwt had similar problems as you mentioned, and after reproducing them (or something related ) I found that the call to os.path.exists that is made from the original os.listdir, was the problem in this case. I'm not really sure if this is a good fix for that.

@mrbean-bremen
Copy link
Member Author

I may have another go at this later and try to reproduce the problem in a unit test.

@mrbean-bremen
Copy link
Member Author

@jmcgeheeiv - I added a test that reproduces the problem. please review!
Note that I rebased against master.

@mrbean-bremen
Copy link
Member Author

As to the explanation why the problem happened in the first place:
Other than the other patched modules, path is actually implemented in two modules: ntpath/posixpath (one of which is imported as path by os) and genericpath, which implements stuff common for both systems. If calling os.path.exists from inside the unit test, the real os module is used as it should be, but it calls the implementation in genericpath, which calls os.stat on its part. If genericpath is not added to SKIPNAMES, this last os will be stubbed out, so stat is called on the fake file system.

@jmcgeheeiv
Copy link
Contributor

Oh, dear. Could you resolve this conflict and merge it?

Well done, @mrbean-bremen.

- allow to add really existing files and directory trees to the fake
file system, with the contents read on demand
- see pytest-dev#170
- added example test
- fixed documentation and formatting
- this seems to fix the problem that using real os.path functions
implemented in this module are stubbed out
- regression test for exception happening if 'genericpath' is not added
to SKIPNAMES
- do not add 'genericpath' to SKIPNAMES if path is not patched
@mrbean-bremen mrbean-bremen merged commit e2f9424 into pytest-dev:master Apr 14, 2017
@mrbean-bremen
Copy link
Member Author

Done - caused that conflict myself. Thanks!

@mrbean-bremen mrbean-bremen deleted the real_file_access branch April 17, 2017 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants