Updates to lastfailed cache not necessarily concurrent?

[xmo-odoo]

I've got two fairly long-running test suites (we're talking a pair of hours each) which share a pytest.ini and a "main" conftest (though they differ in details so running them together is currently not an option).

When running them concurrently I've regularly had issues with --lf sometimes behaving oddly but I've usually chalked it to me missing things (and I usually work on one or the other and thus do not run them concurrently), however recently I've had a situation where I wanted to work on both and thus run them concurrently on the same machine, following which I've had very odd issues I wanted to investigate.

After nuking the cache I ran both suites, one had 18/41 failures and the other 26/120, however only one of the test suites filed the cache, the other one is entirely missing. The failures of the suite which finished last are present in the lastfailed cache, the others are not.

Is it possible that the lastfailed handler reads the cache before starting the run, then writes back from what it originally read rather than re-read the cache before updating and writing it back?

[nicoddemus]

Hi @xmo-odoo,

That's correct, currently the cache is only read at the start of the session, and then written back at the end of the session.

[xmo-odoo]

@nicoddemus I'm not going crazy, that's good news.

Assuming you'd be open to changing this behaviour that would be in cacheprovider right? If I'm reading the code correctly, what lastfailed currently does is:

• read the entries at start
• during run, pop successful entries (if any) and add failures (for non-ff runs I guess)
• re-read entries and if the internally updated entries differ from the re-read ones write them back

I'm thinking the LF plugin could maintain a list of successful tests (not just failed) and then rather than overwrite the cache entirely it would remove the entries which passed (if present) then add those which did not.

Implementation wise, I guess the least disruptive would be to modify the internal lastfailed so that passing tests get mapped to False instead of popped, then during pytest_sessionfinish filter out the keys mapped to False and pop them from the cache we just re-read, then merge the rest into the cache, and write the cache back.

edit: thinking about it for a minute more, this would still re-add entries removed by an other test suite, so LFPlugin should really move its internal passed / failed state outside of the lastfailed it read from the cache (e.g. have two sets and pytest_runtest_logreport and pytest_collectreport would just add entries to the relevant set instead of adding to or popping from self.lastfailed), then use these as "commands" to reapply to the cache it finds at the end.

[nicoddemus]

Hi @xmo-odoo,

Your assessment is correct in general, but keep in mind there are a few questions to consider:

• The cache/lf plugin do their work for every run, if the user wants it or not, so performance here is a concern;
• Any strategy to support this correctly for simultaneous runs would probably involve some kind of file lock, because nothing prevents one run to overwrite the contents of the cache from another run while things are inconsistent.

For those reasons I'm initially -1 on implementing incremental changes which would work "most of the time", but still not be resilient enough. What do you think?

[xmo-odoo]

The cache/lf plugin do their work for every run, if the user wants it or not, so performance here is a concern

I wouldn't expect that to be too much of an issue, even with a huge number of items I'd expect the JSON serialization to dwarf the messing about with failures, though we could probably bench that based on ridiculous loads e.g. have a million test cases and remove 1 / all and/or re-add an other million, something like this.

Any strategy to support this correctly for simultaneous runs would probably involve some kind of file lock, because nothing prevents one run to overwrite the contents of the cache from another run while things are inconsistent.

Aye that's probably fair, create a lockfile (waiting if one exists), load the stored data, do the messing about, save, delete the lockfile.

Is there already clean lockfile support in pytest or one of its dependencies?

For those reasons I'm initially -1 on implementing incremental changes which would work "most of the time", but still not be resilient enough. What do you think?

It's a pain in the ass because it means taking more than 5mn for a quick hack but it's completely fair 😅

[blueyed]

I think it makes even sense to update the cache after each change (new failure, new pass), which allows to run pytest --lf some/subset already in parallel while a full test run is still running.

[The-Compiler]

I think it makes even sense to update the cache after each change (new failure, new pass), which allows to run pytest --lf some/subset already in parallel while a full test run is still running.

That might result in quite a lot of disk I/O, which might slow pytest runs down.

(disclaimer: I haven't read the entire discussion)

[nicoddemus]

That might result in quite a lot of disk I/O

Agree, seems undoable as it would incur reading and writing the entire set each time.

[blueyed]

That might result in quite a lot of disk I/O

Agree, seems undoable as it would incur reading and writing the entire set each time.

Well, this would only get done if there is a new failure or a failure turned into a pass - i.e. still a single read and no write with p --lf without fixing anything.
It could also be limited to e.g. max. 1 update per 5 seconds, or/and with some explicit request, e.g. SIGHUP.

[nicoddemus]

You are right @blueyed, this would not happen all the time, it would be handled by the lf plugin.

Unfortunately we would still have to deal with the locking problem to sort out.

In general, I'm not a big fan of adding complexity to the cache plugin itself, I think it should be kept simple and anything more complex (supporting parallel writes from separate instances for example) is out of scope for it, I believe.

[RonnyPfannschmidt]

we should very carefully choose what pytest should guarantee, then we can decide how to move there

• and if certain guarantees will need extra libraries

[xmo-odoo]

I think it makes even sense to update the cache after each change (new failure, new pass), which allows to run pytest --lf some/subset already in parallel while a full test run is still running.

I don't know that a pseudo-live view would make much sense given the second run wouldn't be picking up new successes or failures, it seems like it'd be pretty confusing.

Agree, seems undoable as it would incur reading and writing the entire set each time.

If the cache format isn't an external / official API, maybe it could be an sqlite file? That should take care of the concurrency issues and (if that's desirable) the writing costs?

[blueyed]

I think it makes even sense to update the cache after each change (new failure, new pass), which allows to run pytest --lf some/subset already in parallel while a full test run is still running.

I don't know that a pseudo-live view would make much sense given the second run wouldn't be picking up new successes or failures, it seems like it'd be pretty confusing.

The use case here is running the whole test suite, then seeing some failures, and being able to run with --lf already on a subset (path), e.g. with --pdb, -x etc.
The use case here would not cause failures to turn into passes, but if you're quick enough to fix something and re-run it, it would be recorded as a pass in the end (since the longer, outer run would have updated for its "failure" already, and the inner then for a "pass").

[Zac-HD]

IMO: a concurrent cache should be a seperate plugin, because it'd very likely come with some performance costs for normal non-concurrent use. Closing per #12465 (comment).