Test MM Core calls with mocks #239
Conversation
|
Preview page for your plugin is ready here: |
There was a problem hiding this comment.
I think this is an excellent step towards improved test coverage, and I think you've made some really excellent choices here. Great job, Ziwen!
I really like that you're testing the io.core_functions first, and I second your idea that we should work towards isolating all of our pycromanager/external calls into io.core_functions. We're likely not far from that goal when we begin full work towards 1.0.0.
I particularly like:
- that you've made the test image width and height random. This isn't exciting yet, but we've seen all sorts of bugs with the user selecting different ROIs (for example, see today's Phase acquisitions fail after changing MM ROI #236) not to mention challenging background correction bugs.
- that you've marked much of the initialization code to be moved to a fixture when we're ready. I think it's great that you're taking this step by step.
I think this is ready to merge since it tests one file cleanly.
What do you think the next file/functionality to test is? I'd be particularly excited to see the calibration code tested since it will not be changing anytime soon, and we have a long list of setting combinations that we'd like to test. I can help you set up simple simulations of the LCs and cameras that will let us do a simulate a complete calibration.
| IMAGE_WIDTH = np.random.randint(1, 2**12) | ||
| IMAGE_HEIGHT = np.random.randint(1, 2**12) |
There was a problem hiding this comment.
Cool! This is a really nice place for randomness.
| IMAGE_HEIGHT = np.random.randint(1, 2**12) | ||
| PIXEL_COUNT = IMAGE_HEIGHT * IMAGE_WIDTH | ||
| # serialized image from the pycromanager bridge | ||
| SERIAL_IMAGE = np.random.randint(0, TIFF_I_MAX, size=(PIXEL_COUNT,)) |
There was a problem hiding this comment.
Fantastic. I think on future iterations we can have a really simple noise model here. No changes needed now.
Codecov Report
@@ Coverage Diff @@
## main #239 +/- ##
==========================================
+ Coverage 10.02% 11.54% +1.51%
==========================================
Files 40 41 +1
Lines 5265 5355 +90
==========================================
+ Hits 528 618 +90
Misses 4737 4737
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I think this makes a lot of sense. One thing I (intentionally) left out in this PR is the generation of test data. I am almost convinced by this paper that we should use Hypothesis to do this, and I think we can have a better understanding of what do we need from such a tool once we start to conceive of testing the most physics-heavy part of |
|
Just skimmed the paper...looks pretty cool. I think it's definitely worth a try. |
|
|
||
| # TODO: move these to fixture or generate with Hypothesis | ||
| # dynamic range | ||
| TIFF_I_MAX = 2**16 |
There was a problem hiding this comment.
I'd recommend also testing with 8-bit and 12-bit images in the future.
|
This is great! Thanks for getting it started @ziw-liu. |
Our test coverage has been declining. This is an initial attempt to address this problem by using simple mock-up objects to unit-test methods that make calls to Micro-Manger APIs.
On a side note, it may be beneficial if we isolated all outbound calls via
pycromanagerand aggregate them in theio.core_functionsmodule.Starting to address #156.