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

Move nf-test files and container definitions #15

Merged
merged 37 commits into from
Dec 6, 2023
Merged

Conversation

drpatelh
Copy link
Member

@drpatelh drpatelh commented Nov 30, 2023

  • Removed container.config and moved container definition back into process
  • Deleted empty nextflow.config files
  • Changed registry info where required
  • Updates CI to run nf-test
  • Remove params.publish_dir_mode from some processes because it seemed to break stuff?

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.

Stuff for future discussions I wanted to write down while I was looking at it, but nothing wrong with the code itself.

nextflow.config Outdated
@@ -58,7 +58,7 @@ profiles {
wave {
wave.enabled = true
wave.strategy = ['conda']
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change this to ['conda', 'container', 'dockerfile', 'spack'] it will use these in that order, so a user could use Wave to copy the existing container without rebuilding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. You want to push that change?

nextflow.config Show resolved Hide resolved
modules/local/seqera_runs_dump/main.nf Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
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.

Just checked and there are lots of errors here about missing files and invalid tests.

@adamrtalbot
Copy link
Contributor

No .nf-tests triggered?

@pinin4fjords
Copy link
Member

pinin4fjords commented Dec 1, 2023

Why the move of the test file? This was a top-level 'pipeline' test (patterned after https://github.com/nf-core/fetchngs/blob/master/tests/main.nf.test etc). The move changes which main.nf is referenced, so there may be other changes to make, no?

@adamrtalbot
Copy link
Contributor

Moved pipeline level test back re: #14 (comment)

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.

It's perfect because I wrote most of it.

@drpatelh
Copy link
Member Author

drpatelh commented Dec 1, 2023

Ok. Tidied things up a little. Things left to do that be good to sort out before the release.

  • Update the MultiQC nf-test on nf-core modules to remove the dependency for FASTQC so we don't have to install the module. Doesn't make sense installing it here
  • Add workflow test to workflows/nf_aggregate/tests

What else was left @pinin4fjords ?

@pinin4fjords
Copy link
Member

What else was left @pinin4fjords ?

Subworkflow test? I'll get on the workflow one anyway

@pinin4fjords
Copy link
Member

pinin4fjords commented Dec 5, 2023

MultiQC module groundwork:

@adamrtalbot adamrtalbot self-requested a review December 5, 2023 11:56
@adamrtalbot
Copy link
Contributor

adamrtalbot commented Dec 5, 2023

We seem to be skipping tests fairly frequently leading to false positive results. I've added it as an issue: #16

For now, be careful when accepting a PR and make sure it's ran all tests.

@pinin4fjords
Copy link
Member

We seem to be skipping tests fairly frequently leading to false positive results. I've added it as an issue: #16

For now, be careful when accepting a PR and make sure it's ran all tests.

Ahh yes, sorry, this was me, and you're right, the strategy makes no sense here.

Copy link
Member

@pinin4fjords pinin4fjords left a comment

Choose a reason for hiding this comment

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

Don't merge yet, mucking about with the tags

.github/workflows/ci.yml Outdated Show resolved Hide resolved
modules/local/plot_run_gantt/tests/main.nf.test Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
nf-test.config Outdated Show resolved Hide resolved
@drpatelh
Copy link
Member Author

drpatelh commented Dec 6, 2023

@drpatelh drpatelh merged commit 817ea2f into main Dec 6, 2023
22 checks passed
@adamrtalbot adamrtalbot deleted the release_fixes branch December 6, 2023 17:55
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.

None yet

4 participants