stephrdev / pytest-isort

py.test plugin to check import ordering using isort
MIT License
40 stars 15 forks source link

Caching does not work with xdist #37

Open GergelyKalmar opened 2 years ago

GergelyKalmar commented 2 years ago

It seems that when running pytest-isort with pytest-xdist the cache does not get populated (.pytest_cache/.../v/isort/mtimes is empty). This means that test are re-run on all files when using distributed test execution, which is not very optimal.

Interestingly the same issue appears with pytest-pylint too (see https://github.com/carsongee/pytest-pylint/issues/161, it is actually much worse there because all tests are executed on all workers due to the way the linting is done), I suppose because both plugins are based on pytest-flake8 (I haven't tested it but I think that plugin has the same issue too).

At the same time pytest-mypy works fine because it relies on the built-in cache of mypy. I'm not yet sure why does the cache not get populated with xdist, but it would be awesome if we could figure it out.

GergelyKalmar commented 2 years ago

The problem seems to be with this line: https://github.com/stephrdev/pytest-isort/blob/master/pytest_isort/__init__.py#L77

It appears that config._isort_mtimes is an empty dictionary when using the plugin together with xdist. This is most likely because pytest_sessionfinish is run in the controller node but the part which adds the entries to _isort_mtimes runs on the workers.

I am not yet sure how to solve this issue properly. Perhaps we can send the mtimes back to the controller and save the values there, but I'm not sure how to do that.

GergelyKalmar commented 2 years ago

We can probably do something similar to pytest-mypy: https://github.com/dbader/pytest-mypy/blob/master/src/pytest_mypy.py#L61

They're basically using pytest_configure_node to pass information from the controller to the workers, we can probably do something similar to get information back from a worker regarding mtimes. It is a pretty major re-work of the plugin to support this though I imagine.

stephrdev commented 2 years ago

Hey @GergelyKalmar,

thank you for your detail investigation on the issue. I think you are right. We would need to re-implement the caching similar to the MyPy plugin.

Sadly, I won't be able to tackle that before January. If you feel like you want to do it, you have my full support on reviewing and merging asap.

GergelyKalmar commented 2 years ago

Hi @stephrdev, no worries! I've ended up building a plugin from scratch because I figured I can create an abstraction above all the plugins which rely on running something against a given source file (such as isort, pylint, pycodestyle or pydocstyle), so this way I've solved the issue with basically all plugins I use at the same time. Unfortunately that code is part of a larger private plugin suite, however, the core of the solution is to use the pytest_testnodedown hook with node.workeroutput to pass data from the workers to the controller. You can use pytest_sessionfinish to set the workeroutput (when you are on a worker node) or update the cache (when you are on the controller node).

I think the pytest-mypy plugin works differently – you will notice that it does not skip files but runs mypy against all of them. This is possible because mypy maintains its own cache so subsequent runs are usually quite fast without skipping.

iurisilvio commented 1 year ago

I tried a solution to this issue and it is merged here already. https://github.com/stephrdev/pytest-isort/pull/48

It is not perfect because you still has a minor race condition, but is a lot better than before.