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

'--cache-clear' option to regenerate the cache folder is missing some files #6290

Closed
luizyao opened this issue Nov 29, 2019 · 7 comments · Fixed by #6296
Closed

'--cache-clear' option to regenerate the cache folder is missing some files #6290

luizyao opened this issue Nov 29, 2019 · 7 comments · Fixed by #6296

Comments

@luizyao
Copy link

@luizyao luizyao commented Nov 29, 2019

Description

Before I used --cache-clear option, my cache folder looked like this:

.pytest_cache/
├── .gitignore
├── CACHEDIR.TAG
├── README.md
└── v
    └── cache
        ├── nodeids
        └── stepwise

2 directories, 5 files

then I add --cache-clear option to re-execute, my cache folder looks like this:

.pytest_cache/
└── v
    └── cache
        ├── nodeids
        └── stepwise

2 directories, 2 files

Three files are missing: .gitignore CACHEDIR.TAG and README.md.

This lets me commit files that I don't want to upload while using git commit.

Question

I check the src/_pytest/cacheprovider.py file in source codes.

    @classmethod
    def for_config(cls, config):
        cachedir = cls.cache_dir_from_config(config)
        if config.getoption("cacheclear") and cachedir.exists():
            rm_rf(cachedir)
            cachedir.mkdir()
        return cls(cachedir, config)

I think this issue is caused by this line: cachedir.mkdir(), it will make cache_dir_exists_already True, so that _ensure_supporting_files function not to be called.

       try:
            if path.parent.is_dir():
                cache_dir_exists_already = True
            else:
                cache_dir_exists_already = self._cachedir.exists()
                path.parent.mkdir(exist_ok=True, parents=True)
        except (IOError, OSError):
            self.warn("could not create cache path {path}", path=path)
            return
        if not cache_dir_exists_already:
            self._ensure_supporting_files()

I don't understand why this operation was added. Is there anything else I don't know about it?

@nicoddemus

This comment has been minimized.

Copy link
Member

@nicoddemus nicoddemus commented Nov 29, 2019

Hi @luizyao,

Thanks for the detailed description, appreciate it.

It seems the problem is just that we are not calling self._ensure_supporting_files() is called after cachedir.mkdir() in for_config. That should be enough to fix the problem.

Would you like to open a PR with that fix?

@luizyao

This comment has been minimized.

Copy link
Author

@luizyao luizyao commented Nov 30, 2019

Hi, @nicoddemus:

I don't think it's a good idea to call an instance method(self._ensure_supporting_files()) in a classmethod(for_config).

How about this:

        try:
            if path.parent.is_dir():
                cache_dir_exists_already = True
            else:
                # cache_dir_exists_already = self._cachedir.exists() 
                # if self._cache is an exist non-empty dir, make cache_dir_exists_already true 
                cache_dir_exists_already = self._cachedir.exists() and next(self._cachedir.glob('*'), None)
                path.parent.mkdir(exist_ok=True, parents=True)
        except (IOError, OSError):
            self.warn("could not create cache path {path}", path=path)
            return
        if not cache_dir_exists_already:
            self._ensure_supporting_files()
@blueyed

This comment has been minimized.

Copy link
Contributor

@blueyed blueyed commented Nov 30, 2019

IIRC the idea is to not re-create the ensuring files if the cache dir exists already (likely more information might be available from git-blame).
I think --cache-clear should not remove the "supporting files" instead.

@blueyed

This comment has been minimized.

Copy link
Contributor

@blueyed blueyed commented Nov 30, 2019

I suggest this:

diff --git i/src/_pytest/cacheprovider.py w/src/_pytest/cacheprovider.py
index db086fc2b..543e292e3 100755
--- i/src/_pytest/cacheprovider.py
+++ w/src/_pytest/cacheprovider.py
@@ -50,6 +50,10 @@ def for_config(cls, config):
         if config.getoption("cacheclear") and cachedir.exists():
             rm_rf(cachedir)
             cachedir.mkdir()
+        if config.getoption("cacheclear"):
+            for d in [cachedir.joinpath("d"), cachedir.joinpath("v")]:
+                if d.exists():
+                    rm_rf(d)
         return cls(cachedir, config)

     @staticmethod
diff --git i/testing/test_cacheprovider.py w/testing/test_cacheprovider.py
index 6bf4f5f19..7fc876ea6 100644
--- i/testing/test_cacheprovider.py
+++ w/testing/test_cacheprovider.py
@@ -272,6 +272,7 @@ def test_3(): assert 0
         )
         result = testdir.runpytest(str(p), "--lf", "--cache-clear")
         result.stdout.fnmatch_lines(["*1 failed*2 passed*"])
+        assert testdir.tmpdir.join(".pytest_cache", "README.md").exists()

         # Run this again to make sure clear-cache is robust
         if os.path.isdir(".pytest_cache"):
@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented Nov 30, 2019

Another approach would be to drop the mkdir after the clear, then the normal setup would make the cache dir

This trades less io in the general case for potentially not having all help files in the cache dir

@nicoddemus

This comment has been minimized.

Copy link
Member

@nicoddemus nicoddemus commented Nov 30, 2019

Another approach would be to drop the mkdir after the clear, then the normal setup would make the cache dir

I like this one.

potentially not having all help files in the cache dir

Not sure, the directory itself won't exist at all, and _ensure_supporting_files will create all files then right?

@nicoddemus

This comment has been minimized.

Copy link
Member

@nicoddemus nicoddemus commented Nov 30, 2019

Went ahead with this in #6296, we can continue the discussion there.

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Dec 1, 2019
vinaycalastry added a commit to vinaycalastry/pytest that referenced this issue Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.