Skip to content

Conversation

@pinin4fjords
Copy link
Collaborator

@pinin4fjords pinin4fjords commented Feb 19, 2025

This is a port of the first part of @adamrtalbot 's work in #499 for the new structure. Also corrected gitpod links to codespaces, and adjusted wording so it's more standalone.

@netlify
Copy link

netlify bot commented Feb 19, 2025

Deploy Preview for nextflow-training ready!

Name Link
🔨 Latest commit 073cdf8
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-training/deploys/67c06668f96a970008bb242f
😎 Deploy Preview https://deploy-preview-516--nextflow-training.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@adamrtalbot
Copy link
Collaborator

No cherry-pick 😢 I will never get credit.

@pinin4fjords pinin4fjords changed the title Nftest side quest nf-test side quest Feb 19, 2025
@pinin4fjords
Copy link
Collaborator Author

pinin4fjords commented Feb 19, 2025

No cherry-pick 😢 I will never get credit.

True, I can try to fix that. I sort of started half-heartedly and got lazy handling conflicts

@adamrtalbot
Copy link
Collaborator

It's not a huge deal!

@pinin4fjords
Copy link
Collaborator Author

It's not a huge deal!

New structure complicated things, but added you as author on the first commit here.

@vdauwera
Copy link
Collaborator

General comment: try to start every sentence on a new line as it makes it easier to review specific bits of text, and will make reviewing further changes easier as well.

Copy link
Collaborator

@vdauwera vdauwera left a comment

Choose a reason for hiding this comment

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

This works well as an introduction to nf-test, and it really helps to have the genomics complexity out of the way.

Couple of recommendations in my review, including adding test cases for something other than identity (specific content would be great) and maybe also covering the 'setup method' case?

adamrtalbot
adamrtalbot previously approved these changes Feb 26, 2025
Copy link
Collaborator

@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.

🎉

mribeirodantas
mribeirodantas previously approved these changes Feb 26, 2025
Copy link
Member

@mribeirodantas mribeirodantas left a comment

Choose a reason for hiding this comment

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

Amazing work, @adamrtalbot and @pinin4fjords ! I'm happy with this PR getting merged, but I would like to share the feeling I still remember having when I started playing with nf-test.

The person who does this material for the first time will finish with the feeling of "Ok, I know what are Nextflow tests now, but I still can't do one 😓".

I think we should spend more time helping people provide real inputs to tests, or using some operators to do something with channel elements. It doesn't have to be complex, just more examples so that the person has more material to work with when they want to write their tests. This could be added later, but I think it's important to add at some point.

PS: Some comments on the setup block too, as @vdauwera mentioned.

Co-authored-by: Marcel Ribeiro-Dantas <mribeirodantas@seqera.io>
mribeirodantas
mribeirodantas previously approved these changes Feb 27, 2025
I added line numbers and line highlights to make it easier to
realise which lines changed from "Before" to "After". When testing
the training, sometimes it wasn't so straightforward to see what
changed and line numbers are very useful when teaching.
@mribeirodantas
Copy link
Member

I added line numbers / line highlights to the code block comparisons. More details in the commit message. Feel free to revert it if you think it's not helpful.

mribeirodantas
mribeirodantas previously approved these changes Feb 27, 2025
@pinin4fjords
Copy link
Collaborator Author

I added line numbers / line highlights to the code block comparisons. More details in the commit message. Feel free to revert it if you think it's not helpful.

I like it!

@pinin4fjords
Copy link
Collaborator Author

PS: Some comments on the setup block too, as @vdauwera mentioned.

I'm not 100% sold on including setup at this stage. It's not really needed until you get properly into the weeds with nf-test, I think it might muddy the waters to introduce too early.

@vdauwera
Copy link
Collaborator

PS: Some comments on the setup block too, as @vdauwera mentioned.

I'm not 100% sold on including setup at this stage. It's not really needed until you get properly into the weeds with nf-test, I think it might muddy the waters to introduce too early.

Yeah on further thought I'd be ok leaving it for an 'intermediate' level add-on to this side quest.

@mribeirodantas
Copy link
Member

Yeah on further thought I'd be ok leaving it for an 'intermediate' level add-on to this side quest.

I agree, but we definitely need the next one! My feeling is that this first piece only gives an idea of nf-test, and is not really applicable to what people may need, even in simple pipelines. And that's fine, imho, as long as we have next step 😆

@mribeirodantas mribeirodantas merged commit e868fc1 into master Feb 27, 2025
8 checks passed
@mribeirodantas mribeirodantas deleted the nftest_bits branch February 27, 2025 15:33
@adamrtalbot
Copy link
Collaborator

My feeling is that this first piece only gives an idea of nf-test, and is not really applicable to what people may need

That's a good thing. This might be the first introduction to testing ever and therefore it should be kept to the what and why without diving into every case. The long tail of testing is enormous and there are many, many different situations, if we wrote guidance for all of them we would go mad. Instead, introduce the key concepts and leave the special cases for side quests.

If I google "how do I run a process before an nf-test", the nf-test setup page is the number 1 hit. So my aim for this module is teach people enough to know what to google or where to move next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants