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.

Sign up for GitHub

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

Jump to bottom

Added class attribute Patcher.patch_path #130

Merged

jmcgeheeiv merged 2 commits into pytest-dev:master from mrbean-bremen:fake_path

Oct 9, 2016

Member

mrbean-bremen commented Oct 8, 2016

can be set to False to avoid patching 'path' (see path is stubbed out too aggressively #53)
added optional argument to TestCase constructor to set it


          Added class attribute Patcher.patch_path

60265c0

- can be set to False to avoid patching 'path' (see pytest-dev#53)
- added optional argument to TestCase constructor to set it

Member Author

mrbean-bremen commented Oct 8, 2016

@jmcgeheeiv: Please check if this sufficient for #53.

jmcgeheeiv suggested changes

View reviewed changes

Contributor

jmcgeheeiv left a comment

This needs a test. Perhaps instead of creating an actual file, you could simply set sys.modules['path'] = None and test whether Patcher changes it.

pyfakefs/fake_filesystem_unittest.py

                       """Creates the test class instance and the stubber used to stub out file system related modules.
                         Args:
                           methodName: the name of the test method (as in unittest.TestCase)
                           additional_skip_names: names of modules inside of which no module replacement shall be done
                              (additionally to the hard-coded list: 'os', 'glob', 'path', 'tempfile', 'io')
+                          path_path: if set to False, 'path' modules will not be stubbed out, and 'path' will be

Contributor

jmcgeheeiv Oct 8, 2016 •

edited

patch_path, not path_path

pyfakefs/fake_filesystem_unittest.py

                       """Creates the test class instance and the stubber used to stub out file system related modules.
                         Args:
                           methodName: the name of the test method (as in unittest.TestCase)
                           additional_skip_names: names of modules inside of which no module replacement shall be done
                              (additionally to the hard-coded list: 'os', 'glob', 'path', 'tempfile', 'io')
+                          path_path: if set to False, 'path' modules will not be stubbed out, and 'path' will be

Contributor

jmcgeheeiv Oct 8, 2016 •

edited

The usage of patch_path is rather obscure, so let's explain the hell out of it:

            patch_path: if False, modules named 'path' will not be patched with the fake 'os.path' module.
               Set this to False when you need to import some other module named 'path', for example,
                   from my_module import path
               Irrespective of patch_path, module 'os.path' is still correctly faked if imported the usual way
               using 'import os' or 'import os.path'.

Repeat this explanation in the docstring for Patcher.

pyfakefs/fake_filesystem_unittest.py

@@ @@ -88,6 +90,9 @@ def __init__(self, methodName='runTest'): @@
                       super(TestCase, self).__init__(methodName)
                       if additional_skip_names is not None:
                           Patcher.SKIPNAMES.update(additional_skip_names)
+                      if not patch_path:
+                          Patcher.patch_path = True
+                          Patcher.SKIPNAMES.remove('path')

Contributor

jmcgeheeiv Oct 8, 2016 •

edited

Reaching inside Patcher like this breaks encapsulation for Patcher. Make patch_path an optional argument to Patcher, thus hiding the implementation details within Patcher.

Then again, using instance methods like __init__() to change class attributes like Patcher.SKIPNAMES means that one instance can change the behavior of other Patcher instances. This is bad.

For backward compatability, we must retain class attribute Patcher.SKIPNAMES, so perhaps add an instance attribute self.skipnames = set(self.SKIPNAMES), then conditionally discard 'path' from self.skipnames.

Member Author

mrbean-bremen Oct 8, 2016

Yes, I didn't like this either. I will have another go at this tomorrow, if I find the time, or next week.

pyfakefs/fake_filesystem_unittest.py

@@ @@ -178,7 +189,7 @@ def _findModules(self): @@
                               continue
                           if 'os' in module.__dict__:
                               self._osModules.add(module)
-                          if 'path' in module.__dict__:
+                          if self.patch_path and 'path' in module.__dict__:

Contributor

jmcgeheeiv Oct 8, 2016 •

edited

Redundant. Is attribute patch_path is needed at all?

Member Author

mrbean-bremen Oct 8, 2016

This is the actual check for the replacement of the path module - the removal from SKIPNAMES may or may not be needed additionally.

pyfakefs/fake_filesystem_unittest.py

@@ @@ -88,6 +90,9 @@ def __init__(self, methodName='runTest'): @@
                       super(TestCase, self).__init__(methodName)
                       if additional_skip_names is not None:
                           Patcher.SKIPNAMES.update(additional_skip_names)
+                      if not patch_path:
+                          Patcher.patch_path = True
+                          Patcher.SKIPNAMES.remove('path')

Contributor

jmcgeheeiv Oct 8, 2016 •

edited

Use set.discard() so that repeated removals will not raise an exception.

pyfakefs/fake_filesystem_unittest.py

@@ @@ -134,6 +139,12 @@ class Patcher(object): @@
                   # it appears that adding  'py', 'pytest', '_pytest' to SKIPNAMES will help
                   SKIPNAMES = set(['os', 'path', 'tempfile', 'io'])
+                  # To avoid stubbing out path because another top=level module named path is present

Contributor

jmcgeheeiv Oct 8, 2016 •

edited

I incorporated this excellent explanation into the patch_path docstring above.


          Changes after review by @jmcgeheeiv

3ec0aaf

- use instance instead of class attributes
- enhanced documentation
- added test

Member Author

mrbean-bremen commented Oct 9, 2016

@jmcgeheeiv: Please check if the changes and the test are ok,

jmcgeheeiv merged commit b4380f5 into pytest-dev:master

Contributor

jmcgeheeiv commented Oct 9, 2016 •

edited

Fixed #53.

jmcgeheeiv mentioned this pull request

path is stubbed out too aggressively #53

Closed

mrbean-bremen deleted the fake_path branch

October 10, 2016 17:22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment