Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✅ Upgrade remote sample fixtures #640

Merged
merged 14 commits into from
Jul 20, 2023
Merged

✅ Upgrade remote sample fixtures #640

merged 14 commits into from
Jul 20, 2023

Conversation

blaginin
Copy link
Collaborator

@blaginin blaginin commented Jul 16, 2023

This PR addresses part of issue #603 and improves fixtures in tests:

  • Some tests were using _fetch_remote_sample even though tests have a special remote_sample wrapper.
  • remote_sample previously created a new folder for each run (data-0, data-1, data-2, and so on). Now it uses just one folder for all remote fixtures during a test session.
  • To prevent duplicated downloads, I added download locks to the fixtures.
  • I combined _fetch_remote_sample and download_data since they had the same logic.

@blaginin blaginin self-assigned this Jul 16, 2023
@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Merging #640 (4818c49) into develop (3614f61) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #640      +/-   ##
===========================================
- Coverage    99.78%   99.78%   -0.01%     
===========================================
  Files           65       65              
  Lines         7075     7071       -4     
  Branches      1397     1397              
===========================================
- Hits          7060     7056       -4     
  Misses           7        7              
  Partials         8        8              
Impacted Files Coverage Δ
tiatoolbox/models/dataset/info.py 100.00% <ø> (ø)
tiatoolbox/data/__init__.py 100.00% <100.00%> (ø)
tiatoolbox/models/architecture/__init__.py 100.00% <100.00%> (ø)
tiatoolbox/utils/misc.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@blaginin blaginin changed the title ✅ Upgrade remote sample fixture ✅ Upgrade remote sample fixtures Jul 17, 2023
@blaginin blaginin marked this pull request as ready for review July 18, 2023 10:26
@blaginin blaginin requested a review from shaneahmed July 18, 2023 10:45
@shaneahmed
Copy link
Member

@blaginin Please try to run ruff #625 on this PR to fix a few issues with formatting code structure.

@@ -2281,7 +2281,9 @@ class TestReader:
"type",
COLOR_DICT,
),
"base_wsi": WSIReader.open(_fetch_remote_sample("svs-1-small")),
"base_wsi": WSIReader.open(
_fetch_remote_sample("svs-1-small")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not use remote_sample here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote code a bit but yes. On the plus side, no need to use pytest_generate_tests — everything is happening here, using just native fixtures.

@shaneahmed
Copy link
Member

@blaginin Looks good to me. Just added a couple of minor suggestions.

@shaneahmed
Copy link
Member

shaneahmed commented Jul 19, 2023

The new tests fixture has reduced coverage for dicom and AnnotationRenderer
https://app.codecov.io/gh/TissueImageAnalytics/tiatoolbox/pull/640/indirect-changes
image

@shaneahmed shaneahmed added the dev tools Changes/Updates in Development tools label Jul 19, 2023
@shaneahmed shaneahmed added this to the Release v1.5.0 milestone Jul 19, 2023
@blaginin blaginin requested a review from shaneahmed July 19, 2023 18:57
@shaneahmed shaneahmed merged commit 15f78a0 into develop Jul 20, 2023
@shaneahmed shaneahmed deleted the tests-fixtures branch July 20, 2023 09:28
@blaginin blaginin mentioned this pull request Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev tools Changes/Updates in Development tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants