syrupy: Incompatible with pytest-xdist / thread safety

Heya 👋

Just wanted to share some hacky ideas in case they can be of help:

(1) Could it be possible to circumvent the issue altogether by having one file per test that requests the snapshot fixture?

I understand that this would be a breaking change; this being said, a migration path that reads existing snapshots files and splits them to the new file-per-test would probably be easier than exploring the innards of the pytest-xdist plugins.

The JSON extension works like this but I suspect it also doesn’t work too well with pytest-xdist. There’s a use case for 1 file per test case, but I can see good arguments against it, for example consider the use of parametrization. You probably don’t want 1000s of snapshot files per test case. Especially since these are expected to be committed to version control.

I’m trying my best to see if syrupy can be refactored/improved to the point where minimal work is required for pytest-xdist compatibility. Using this as an opportunity to also cleanup the architecture a bit.

(2) Spawn a python process using the multiprocessing python lib and using that process to coordinate writing. That way you can treat whatever happens in the tests as a black box and just write when all the listeners are done.

pytest-xdist already has the concept of a controller process. I’m hoping to piggyback on this. We’ll see. Whatever we do to ensure pytest-xdist support, it shouldn’t negatively impact our non-xdist users.

Ngl, it seems to me that the python testing tools ecosystem is fragmented to a point where it hinders the maintenance and stabilization of a rich & coherent suite of tools like in Javascript. Treating other plugins as a black box to me seem like a way to reduce headaches.

Syrupy itself doesn’t leverage any “hacks” to do its job. It 100% respects the pytest plugin API. It’s pytest-xdist which is doing some odd things to add parallelization to pytest. This breaks some assumptions when working with the pytest API. This is why we can’t just treat pytest-xdist as a black box, because it does in fact change the way tests are run.

Taking a stab at this over the weekend! ⚒️

@mcataford you’ll want to work off of the next branch. I was running into performance issues which will need to be tackled first. We’re running benchmarks against the main branch, so you can use those benchmarks as a reference.

Will try take a deeper look this weekend but what needs to be clarified:

• How do we know when xdist is done and wrapping up? Is there a pytest hook?
• How can we communicate the snapshots we want to write to a single worker/controller? Is there a built in communication mechanism?

If I can’t solve it immediately, I’ll do some refactoring in syrupy to hopefully make it easier to reason about. I’ve been meaning to do some serious refactoring to the syrupy internals for a while and have been planning it for a v2 release since it’ll affect plugin development.

I started digging into the xdist implementation, and this won’t be as straightforward as I hoped. Definitely open to help/contributions here. It looks like each worker manages its own session. So where/when should we write snapshots? Can two workers get nodes from the same test file? In which case, we might have 2 workers trying to write to the same file?

Even if we overcame that obstacle, we run into a problem with unused snapshot detection. We need to somehow defer our snapshot analysis until after all the workers have been cleaned up.

I haven’t had a chance to work on this just yet. I previously refactored some code to boost performance. Buffering the writes should still be a feasible fix here.

If you’re interested in taking this on, I think we’d want to batch and then defer this operation https://github.com/tophat/syrupy/blob/ec4cbac3fa71eb7dad276005f9433ce26c2f1f9a/src/syrupy/assertion.py#L197 (the write_snapshot call), collect them for after the tests finish running, and flush them out to disk at the same time in some coordinated fashion (still need to look into pytest-xdist to see how that may be possible). The hooks are located in the __init__.py file. – If you are interested, feel free to join our discord and I’ll be available as a resource. Otherwise, I will get to this at some point. I ended up prioritizing the JSON extension over this issue, but as that’s out of the way, this is definitely the next most pressing issue.

Might actually take a stab at it now.

I’m also using pytest-xdist and want to use syrupy in my test suite. I run into a different problem.

Just having syrupy installed will break pytest when using the pytest-xdist option --looponfail.

-f, --looponfail       run tests in subprocess, wait for modified files and re-
                       run failing test set until all pass.

If I uninstall syrupy the error goes away. I have not added any syrupy-specific code to any of my tests.

$ pytest --looponfail
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1400, in _save
    dispatch = self._dispatch[tp]
KeyError: <class 'abc.ABCMeta'>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/bin/pytest", line 8, in <module>
    sys.exit(console_main())
  File "/usr/local/lib/python3.9/site-packages/_pytest/config/__init__.py", line 185, in console_main
    code = main()
  File "/usr/local/lib/python3.9/site-packages/_pytest/config/__init__.py", line 162, in main
    ret: Union[ExitCode, int] = config.hook.pytest_cmdline_main(
  File "/usr/local/lib/python3.9/site-packages/pluggy/_hooks.py", line 265, in __call__
    return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
  File "/usr/local/lib/python3.9/site-packages/pluggy/_manager.py", line 80, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/usr/local/lib/python3.9/site-packages/pluggy/_callers.py", line 60, in _multicall
    return outcome.get_result()
  File "/usr/local/lib/python3.9/site-packages/pluggy/_result.py", line 60, in get_result
    raise ex[1].with_traceback(ex[2])
  File "/usr/local/lib/python3.9/site-packages/pluggy/_callers.py", line 39, in _multicall
    res = hook_impl.function(*args)
  File "/usr/local/lib/python3.9/site-packages/xdist/looponfail.py", line 35, in pytest_cmdline_main
    looponfail_main(config)
  File "/usr/local/lib/python3.9/site-packages/xdist/looponfail.py", line 45, in looponfail_main
    remotecontrol.loop_once()
  File "/usr/local/lib/python3.9/site-packages/xdist/looponfail.py", line 115, in loop_once
    self.setup()
  File "/usr/local/lib/python3.9/site-packages/xdist/looponfail.py", line 77, in setup
    self.channel = channel = self.gateway.remote_exec(
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway.py", line 135, in remote_exec
    gateway_base.dumps_internal((source, file_name, call_name, kwargs)),
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1371, in dumps_internal
    return _Serializer().save(obj)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1389, in save
    self._save(obj)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1407, in _save
    dispatch(self, obj)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1494, in save_tuple
    self._save(item)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1407, in _save
    dispatch(self, obj)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1490, in save_dict
    self._write_setitem(key, value)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1484, in _write_setitem
    self._save(value)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1407, in _save
    dispatch(self, obj)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1490, in save_dict
    self._write_setitem(key, value)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1484, in _write_setitem
    self._save(value)
  File "/usr/local/lib/python3.9/site-packages/execnet/gateway_base.py", line 1405, in _save
    raise DumpError("can't serialize {}".format(tp))
execnet.gateway_base.DumpError: can't serialize <class 'abc.ABCMeta'>

I’m re-examining where we do the reads/writes in an effort to solve https://github.com/tophat/syrupy/issues/539. I’m hoping that work will also solve the concurrency issue with pytest-xdist.