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

add docs to commit new files before pytest #2316

Merged
merged 3 commits into from
Jun 16, 2023

Conversation

mirpedrol
Copy link
Member

@mirpedrol mirpedrol commented Jun 12, 2023

The command nf-core modules test runs tests using pytest. We use the pytest argument --git-aware.
This argument is used to avoid copying the whole .git directory and files ignored by git.
It will only include files listed by git ls-files.
In order to test a new module or subworkflow, the new files have to be committed, otherwise they will be ignored. This was reported in #2235 and nf-core Slack.
This PR adds documentation to advise committing changes before running the command, as it is better to keep the argument --git-aware.

It throws a warning if there are uncommitted changes:
image

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

@mirpedrol mirpedrol requested a review from mashehu June 12, 2023 14:04
Copy link
Contributor

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

Nice! we could also print a warning if we see uncommited modules or do you think that is not that easy to check?

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #2316 (b4e8f82) into dev (1e39535) will decrease coverage by 0.13%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##              dev    #2316      +/-   ##
==========================================
- Coverage   73.05%   72.93%   -0.13%     
==========================================
  Files          78       78              
  Lines        8732     8763      +31     
==========================================
+ Hits         6379     6391      +12     
- Misses       2353     2372      +19     
Impacted Files Coverage Δ
nf_core/components/components_test.py 52.80% <14.28%> (-3.29%) ⬇️

... and 2 files with indirect coverage changes

@mirpedrol
Copy link
Member Author

We don't use git in this command, so we would need to import GitPython, but we already have it in requirements for the sync command :) I will try to add the warning

@MatthiasZepper
Copy link
Member

MatthiasZepper commented Jun 12, 2023

Nice! we could also print a warning if we see uncommited modules or do you think that is not that easy to check?

One can definitively check if there are uncommitted changes in the whole working tree with Repo.is_dirty(). Might be a bit more complicated to restrict it to modules only, but is_dirty() accepts os.PathLike argument, so might be feasible.

@@ -179,6 +180,11 @@ def _run_pytests(self):
console = rich.console.Console()
console.rule(self.component_name, style="black")

# Check uncommitted changed
repo = Repo(self.dir)
if repo.is_dirty():
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set an option to ignore this? Sorry for being particular 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

It is only a warning, do we need to ignore it? Maybe better to capture possible errors, so it doesn't fail, in case someone doesn't have a repo or something similar

@mirpedrol mirpedrol merged commit 37c3d06 into nf-core:dev Jun 16, 2023
@mirpedrol mirpedrol deleted the test-git-aware branch June 16, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants