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.

Sign up for GitHub

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

Jump to bottom

Use paths instead of tags as matrix variable #300

Merged
merged 12 commits into from
Mar 13, 2024
Merged

Use paths instead of tags as matrix variable #300

merged 12 commits into from
Mar 13, 2024

Conversation

adamrtalbot
Copy link
Contributor

@adamrtalbot adamrtalbot commented Mar 4, 2024 •
edited
Loading

Based on nf-core/phageannotator#97

See related nf-core/phageannotator#99

Additions:

  • find_changed_files returns a list of paths instead of tags
  • Uses this as input to matrix

Benefits:

  • More efficient testing (1 test per file).

Additional additions:

  • include.yaml file to specify files to include or exclude from test sets
  • Will include additional tests if major files have been modified, e.g. the .github/workflows/* files
  • Uses globbing for exclude statement to match multiple files.
  • Tag matching is case insensitive and replaces / with _ so our tags are backwards compatible.

Changes:

  • bump setup-nextflow
  • Adds pyyaml as dependency
  • Uses pathlib.Path object where possible to prevent coercion back and forth between strings
  • Splits find_files into two functions, find_changed_files and detect_nf_test_files

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/fetchngs branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@adamrtalbot
Use paths instead of tags as matrix variable
0fc43a9 
Additions:
 - find_changed_files returns a list of paths instead of tags
 - Uses this as input to matrix

Benefits:
 - More efficient testing (1 test per file).

Additional additions:
 - include.yaml file to specify files to include or exclude from test sets
 - Will include additional tests if major files have been modified, e.g. the .github/workflows/* files
 - Uses globbing for exclude statement to match multiple files.

Changes:
 - bump setup-nextflow
 - Adds pyyaml as dependency
 - Uses pathlib.Path object where possible to prevent coercion back and forth between strings
 - Splits find_files into two functions, find_changed_files and detect_nf_test_files
@adamrtalbot
prettier
c92b31b
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 4, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 18b5627

+| ✅ 155 tests passed       |+
#| ❔   5 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

  • files_exist - File not found: assets/multiqc_config.yml
  • files_exist - File not found: conf/igenomes.config
  • files_exist - File not found: .github/workflows/awstest.yml
  • files_exist - File not found: .github/workflows/awsfulltest.yml

❔ Tests ignored:

  • files_exist - File is ignored: conf/modules.config
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/fetchngs/fetchngs/.github/workflows/awstest.yml
  • multiqc_config - 'assets/multiqc_config.yml' not found

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-03-13 11:31:33

CarsonJM
CarsonJM approved these changes Mar 6, 2024
View reviewed changes
Copy link

@CarsonJM CarsonJM left a comment

Choose a reason for hiding this comment

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

LGTM! I'll update phageannotator's to match this

@adamrtalbot
Changes:
fab42ea 
 - Better logic for detecting changed files and excluding the ignore list
 - Break out include statement into separate function
 - Correctly identify changed files in the same dir as changed files
 - Better name for function than 'generate'
@adamrtalbot
Code re-use so we don't duplicate functions everywhere
3e2665f
@adamrtalbot
Make tags check case insensitive
3070255
adamrtalbot added a commit to adamrtalbot/rnaseq that referenced this pull request Mar 11, 2024
@adamrtalbot
Add new testing strategy based on nf-test files
5b62627 
Changes:
 - Workflow now parses repo for changed files with Python
 - Runs relevant tests by listing their nf-test files as the matrix of values
 - Uses tags to check for dependencies only

Related to:
nf-core/phageannotator#97
nf-core/fetchngs#300
@adamrtalbot adamrtalbot mentioned this pull request Mar 11, 2024
Merged
11 tasks
maxulysse
maxulysse reviewed Mar 13, 2024
View reviewed changes
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

Love it

@adamrtalbot adamrtalbot merged commit d9d54b8 into nf-core:dev Mar 13, 2024
62 checks passed
This was referenced Mar 18, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse left review comments

@CarsonJM CarsonJM CarsonJM approved these changes

@drpatelh drpatelh Awaiting requested review from drpatelh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamrtalbot @maxulysse @CarsonJM