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

Fake Filesystem Never Empty. There Is Always Folder '/var/folders' on MacOS #329

Closed
Quibi opened this issue Nov 14, 2017 · 11 comments
Closed

Comments

@Quibi
Copy link

Quibi commented Nov 14, 2017

Before pyfakefs 3.3 (3.2 and above), it is possible to check that the fake filesystem is empty and only has the root directory /. A simple scandir() was enough.

However, since version 3.3 the fake filesystem is not really empty any more. There is always a '/var/folders' directory. Why?

I think that a best solution —the ideal one, I must say— will be that the fake filesystem is really empty, except for root directory. Any file or folder must exist only if you create it. That was the behaviour of pyfakefs till 3.3.

In my case, it causes a lot of errors in my tests. scandir, ListDir, glob, etc. does not returns expected results any more...

Pyfakefs 3.3
Python 3.6.3
MacOs 10.6.2

Thanks a lot

@jmcgeheeiv
Copy link
Contributor

Test testNewFilesystem() in fake_filesystem_test.py checks that the new file system is empty.

Further, I searched for "var" and "folders" in the source code and found nothing unusual.

Could you please provide code that demonstrates the problem? Thank you.

@mrbean-bremen
Copy link
Member

mrbean-bremen commented Nov 15, 2017

That is due to the fact that the temp directory is now always created in setUp() (under MacOS it is located under /var/folders). The reason for this is a technical one - the tempfile module is no longer faked out (the fake implementation was incomplete anyway), instead the real module is used with the fake file system. The real tempfile module relies on the existence of the temp directory, as may some other modules (see #304 for an example), so we now create it in setUp(). I didn't realize that this could break existing tests, so it is not mentioned specifically in the release notes.

I still think that this is more expected behavior than a bug, but you are right of course, that it breaks tests relying on the old behavior. I can think of 2 solutions, both not ideal: add some function in pyfakefs that changes the behavior back to not creating the temp path (could break tests that use temofile), or provide an additional function that checks for an empty file system (needs more changes to the existing tests). If you are using fake_filesystem_unittest, you could also use your own derived test class, that removed the temp path in setUp().

What do you think?

And thanks for the fast report BTW, with 3.3 only just released!

@jmcgeheeiv
Copy link
Contributor

Aha.

@Quibi
Copy link
Author

Quibi commented Nov 15, 2017

Thanks @mrbean-bremen. I think that you are right.

I had written additional comments on this issue (providing an example and some outputs). However, after rereading your answer (and better understanding it), I have deleted those comments. Your answer fully explains the issue in my opinion.

I cannot say that this (new) expected behaviour is the best one for a package mocking the filesystem. I rather prefer a mock that only creates those files and folders that are commanded to do.

I understand that there are some trade-offs and some implementation decisions that, of course, I am not able to catch as a seasonal programmer.

So it is fine. I will refactor my tests to fulfil the new behaviour.

Thanks you and @jmcgeheeiv for this nice and useful package.

@Quibi Quibi closed this as completed Nov 15, 2017
@mrbean-bremen
Copy link
Member

I had a look at your results, and I have no idea, where these files come from - this seems to be a location where some caches are stored under MacOS (not the temp directory, actually), and someone must be writing them after setupPyfakefs(), but I can't understand who from your code. You can always set a breakpoint in the file creation code in pyfakefs to check where that comes from.
Anyway, I think it is not a good idea to assume that you have full control over the contents of the fake file system, as nobody prevents some python libraries to create files in the system cache or temp directories that then end up in the fake file system.

@Quibi Quibi changed the title Fake Filesystem Never Empty. There Is Always Folder '/var/folders' Fake Filesystem Never Empty. There Is Always Folder '/var/folders' on MacOS Nov 15, 2017
@Quibi
Copy link
Author

Quibi commented Nov 15, 2017

I think it is not a good idea to assume that you have full control over the contents of the fake file system, as nobody prevents some python libraries to create files.

I have not thought about that. You are right: the filesystem can be written by other modules as well. Nobody should expect that a fake filesystem is still empty after a while because of that.

However, in my tests, I usually check that the filesystem is empty just after "creating" the fake filesystem or immediately after deleting all files. So I think that the problem that you mentioned does not apply here, in my tests. Maybe I am wrong about that assumption.

On the other hand, I have made the "breakpoint test". The "results" are:

  • temp_dir is set to '/var/folders/_g/00pdg6k10y90x6g2wfp6z8h00000gn/T' (That's correct, I suppose).
  • at fake_filesystem_unittest.py:402, the value of self.fs.root.contents = {} (So filesystem is empty at that point: Patcher.SetUp() just after calling _refresh method.)
  • after fake_filesystem_unittest.py:426, the temp directory has been created. So self.fs.root.contents is no longer empty and contains the folder /var.

Does this provide useful information to you? Should I perform any additional debugging to get more data?

@mrbean-bremen
Copy link
Member

Ok, thank you - I wasn't aware that the temp folder location under MacOS is that cryptic (certainly hasn't been this way the time I have worked with it).
I understand your problem, but I still think it would be better to restrict your tests to the part of the file system you are working with. The first test you make is basically just testing that pyfakefs is working - that should not be your job ;) And if you are testing anything using pyfakefs, the location you will be testing should be the same as in the real fs - so it usually makes no sense to check the content of / (or /var, for that matter).
What do you think?

@Quibi
Copy link
Author

Quibi commented Nov 15, 2017

I really do not know so much about /var folder on MacOs. I have found this article: “What is "/var/folders"?” by Magnusviri with some useful details.

On the other matter, about the way I use pyfakefs, well... I have coded a home-made library for my own use in several personal projects. Maybe not the best approach, but has worked for me for ages.

As well as in many projects around, this library has its own history and its own sins.

Checking that pyfakefs was "running" comes from the developing process. At first, I could not manage to make the package work. So I checked that all things were in place. That was long time a ago, before fake_pathlib was implemented and I had to crafted my own mock based on pyfakefs. The feature is still there, in my home-made library, and I found it very useful on my projects.

For instance, in this particular case, I use it to check that pyfakefs is working since the /var folders were duplicated in fake and real filesystem.

Call it: amateur's insecurity.

The library "subclasses" pyfakefs provides additional services and that the reason behind some decisions.

For example, checking if the filesystem is empty or not was very useful for subtests. I can "re-start" the filesystem just by deleting all contents and checking if it is empty to assert that everything is fine. At the beginning, I had many inconveniences because of side-effects and checking that the fake filesystem is empty killed all troubles.

And so on... As I said, there would be better approaches for sure.

Thanks a lot.

@Quibi
Copy link
Author

Quibi commented Nov 16, 2017

Sorry for resurrecting this issue. I have made some further investigation and I would like to propose some minor changes in the package.

First of all, I would like to say that an initial empty fake filesystem is a desired feature for me. So I have tried to revert the actual behaviour to previous one in versions above 3.3. If you consider that this is not a desirable feature, this patch is worthless.

My proposal is to delay the creation of the fake temporary directory until it is actually requested. To achieve this, I propose to stub the tempfile._sanitize_params() method which is called, among other things, to set the default directory where a temporary folder or file would be created.

The stub method _sanitize_params checks if the temporary directory already exists, and if not, created.

The proposed alternative passes the tests of pyfakefs package.

I have made modifications in fake_filesystem_unittest.py and in fake_filesystem_unittest_test.py. The former was modified to include the hack while the latter includes an additional test for checking that initial filesystem is empty (not really needed).

Both files can be seen at https://gist.github.com/Quibi/05ba9afeb94994123dcfd0bb1e7bbb1f.

Changes can be searched by '# HACK' for convenience.

In my opinion, the advantages of this approach are:

  • Initial empty fake filesystem.

  • Fake temporary directory is created just once needed.

  • No need to mock all tempfile package as proposed in Remove tempfile from Patcher #191

  • Since tempdir.gettempdir() is not stubbed, the user can delete and re-create the temporary directory as it would be in his own OS when ever he wants.

The disadvantages I can imagine are:

  • A slightly complicated if compared with correct implementation. Among other things, it needs to create and use a flag called __create_temp_dir to avoid that the fake temporary directory was created more than once (e.g. after being deleted the temporary directory and call for a new temporary file —in this case, a test should fail, for instance). This is not nice.

  • Additional tests on tempdir may be needed. Actual tests are passed and I carefully scanned the source of tempfile, but I can assure that this hack will work in any possible situations (instead, the current implementation does).

  • No temporary directory in fake filesystem if expected (minor since that was not the expected behaviour in previous version).

Thank you.

@mrbean-bremen
Copy link
Member

That sounds reasonable to me. I currently don't have a working PC at home (my hard disk finally died today), so I can't check this very well - but you may just create a pull request and we can go from there. It will take me a few days to get back to that, so take your time ;)

@Quibi
Copy link
Author

Quibi commented Nov 16, 2017

Ouhh. Hard drives... Sorry about that.

My first pull request. It will be a great new experience...

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

No branches or pull requests

3 participants