Codecov Report

Attention: Patch coverage is 77.50000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 80.80%. Comparing base (a6f819f) to head (81085b6).
Report is 17 commits behind head on main.

✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main     #490      +/-   ##
==========================================
+ Coverage   78.55%   80.80%   +2.25%     
==========================================
  Files         182      187       +5     
  Lines       15933    16615     +682     
  Branches     2100     2184      +84     
==========================================
+ Hits        12516    13426     +910     
+ Misses       2717     2443     -274     
- Partials      700      746      +46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This is really cool, Jérémy. Looking forward to reviewing this, when you're ready.

What do you think of this way of doing things? @Andrewwango

@jscanvic thanks for doing this hard work. I really agree with this point (and it is not just true of datasets, but also mocking other things e.g. external libraries, maybe even long-running algorithms.)

write tests that would pass for the actual data, and then mock the data to make them pass, and not to start by mocking the data before cobbling implementation-dependent tests around it

It would be worth adding best-practice instructions somewhere in the repo about how we write tests - i.e. write tests for the intended outcome of the code, then writing code to pass these, rather than writing code then writing tests that pass on the code.

AFAIK the remaining TODOs for #487 wrt. datasets are:

• Add mocking for all datasets
• Add --mock as an option? and then that is default in the CI
• Add a note in the dataset docs for "contributing a dataset" that you can mock it but we really trust you to make sure your datasets actually pass before mocking it

Thanks @Andrewwango

Do we want to mock the datasets that are currently downloaded just fine on the CI server? It is not clear to me. I think all of the datasets are now tested, mocked or not (though coverage can still be increased).

I added an option for mocking using an environment variable - it gets less in the way in my opinion.

I'll update the contributing guide later.

Mocking other datasets makes it more uniform I guess, but you're right that there's no point to mocking them if we also want to test downloading them.

Thanks for having taken the time to review this PR @Andrewwango!

Following what @romainvo does in #474, I changed some of the calls to patch to calls to patch.object. The difference between the two lie in the way the patched object is referenced:

patch("os.listdir", return_value=[f"{i}_HR.png" for i in range(1, 15)])
patch.object(os, "listdir", return_value=[f"{i}_HR.png" for i in range(1, 15)])

For bare patch, the object is referenced by its import path, but for patch.object it's referenced through a variable directly. It should make static analysis easier and it should be more convenient with existing tooling, e.g., for using the going to definition/implementation in code editors.

Thanks Andrew! I made the changes