napari / napari

napari: a fast, interactive, multi-dimensional image viewer for python
https://napari.org
BSD 3-Clause "New" or "Revised" License
2.1k stars 410 forks source link

Move `_mock_app` fixture to `_testsupport.py` and add it to `make_napari_viewer` #6823

Closed DragaDoncila closed 2 months ago

DragaDoncila commented 2 months ago

References and relevant issues

See zulip thread for discussion.

Description

Prior to this PR, running tests with the make_napari_viewer fixture outside of napari was failing due to shared instances of the NapariApplication. In internal tests, we autouse the _mock_app fixture, so each test had its own new instance.

To make sure make_napari_viewer behaves the same in our internal tests as it does externally, this PR moves the _mock_app fixture to the same file as make_napari_viewer and adds _mock_app to make_napari_viewer explicitly. We also remove the autouse flag in favour of explicitly adding _mock_app to tests that require the NapariApplication - note that tests using make_napari_viewer will still use to the mock application without needing to be changed.

DragaDoncila commented 2 months ago

Hmm looks like a real failure - will take a look and ping for reviews after I fix it

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.39%. Comparing base (ef84d01) to head (bca7429). Report is 6 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6823 +/- ## ========================================== + Coverage 92.38% 92.39% +0.01% ========================================== Files 615 614 -1 Lines 54907 54904 -3 ========================================== + Hits 50724 50728 +4 + Misses 4183 4176 -7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

psobolewskiPhD commented 2 months ago

I can confirm this fixes the issue I was having, documented in the zulip thread.

DragaDoncila commented 2 months ago

The errors we're seeing are only occurring on npe2 0.7.2 and 0.7.3, but not the latest two releases. The tests fail consistently when run in the terminal, but I can't get any of them to fail when debugging, to figure out exactly what's happening. I'm going to bump the npe2 min req to 0.7.4 :woman_shrugging: Worth noting too this is an error in the cleanup of the DynamicPlugin used for testing, so I really don't think it's worth the time to track it down exactly.

lucyleeow commented 2 months ago

Clutching at straws, do you think https://github.com/napari/npe2/pull/325 could be related? (nevermind, it was added in 0.7.3, but tests fail 0.7.2 as well)

lucyleeow commented 2 months ago

It would be nice to follow up with an update to the app-model docs, to say that _mock_app is not autouse: https://napari.org/dev/developers/architecture/app_model.html#mock-app

DragaDoncila commented 2 months ago

@lucyleeow good point have opened docs/#403 to address