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

Download: Fix unintentional downloading of containers in test #2434

Merged

Conversation

MatthiasZepper
Copy link
Member

@MatthiasZepper MatthiasZepper commented Sep 22, 2023

When I initially wrote the tests for the new nf-core download --tower functionality, there was little test coverage regarding container image detection. Therefore, I incorporated some functionality along in this line into a new test test_download_workflow_for_tower().

Since I felt that testing everything with a real pipeline was preferable over a dummy pipeline with just one or two nicely formatted modules, I chose rnaseq as a test subject. Since the test was never intended to actually download any of the detected images, I was not bothered about the pipeline comprising more than 30 modules.

Initially, the test also performed as intended. But already two weeks later, we decided that downloading containers by default would improve the UX of nf-core download. Therefore, downloading containers was no longer optional when choosing the --tower download method.

self.container = "singularity" if singularity_cache_index or bool(tower) else container

This small change had unintended side effects in the tests, because now the test would actually download the 33 containers and thus several GB of data. This bug was noticed recently by @adamrtalbot and @awgymer (Thanks!).

Back then, I was not aware that it is possible to mock objects in tests. Therefore, instead of testing a function that also performs undesirable steps, I put a similar function in the test that resembled the original function, but omitted the unwanted steps. Since then, I learnt some basic mocking capability and changed the test to mock the download function now. This should ensure that no images are actually pulled, but will inevitably decrease test coverage slightly.

Since the actual download of a container image is tested with a different test test_singularity_pull_image_singularity_installed(), which I have written after the Tower functionality was first added, just mocking the download in test_download_workflow_for_tower() should be safe.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@MatthiasZepper MatthiasZepper marked this pull request as ready for review September 22, 2023 15:22
@mirpedrol mirpedrol added this to the 2.10 milestone Sep 22, 2023
Copy link
Contributor

@adamrtalbot adamrtalbot left a comment

Choose a reason for hiding this comment

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

Looks great!

@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Merging #2434 (7592320) into dev (6b20a81) will decrease coverage by 0.39%.
Report is 24 commits behind head on dev.
The diff coverage is 20.83%.

@@            Coverage Diff             @@
##              dev    #2434      +/-   ##
==========================================
- Coverage   71.07%   70.69%   -0.39%     
==========================================
  Files          87       87              
  Lines        9431     9431              
==========================================
- Hits         6703     6667      -36     
- Misses       2728     2764      +36     
Files Changed Coverage Δ
nf_core/download.py 54.70% <20.83%> (-4.65%) ⬇️

@mirpedrol mirpedrol merged commit 4570ea4 into nf-core:dev Sep 23, 2023
20 checks passed
@MatthiasZepper MatthiasZepper deleted the prevent_pytest_container_download branch February 21, 2024 10:15
@edmundmiller edmundmiller added the download nf-core download label Jun 13, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
download nf-core download
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants