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

Update nf-tests and language server fixes #153

Merged
merged 34 commits into from
Nov 11, 2024

Conversation

LouisLeNezet
Copy link
Collaborator

@LouisLeNezet LouisLeNezet commented Oct 31, 2024

  • Fix image in usage.md.
  • Fix small warnings and errors with updated language server.
  • def has been added when necesary, `
  • :use instead of,` in assertions,
  • _ added to variables not used in closures,
  • for loop replaced by .each{},
  • remove unused code / input.
  • Fix getFileExtension function.

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/phaseimpute branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines 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).

@LouisLeNezet LouisLeNezet self-assigned this Nov 5, 2024
@LouisLeNezet LouisLeNezet added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request labels Nov 5, 2024
@LouisLeNezet LouisLeNezet added this to the v0.99.0 milestone Nov 5, 2024
@LouisLeNezet LouisLeNezet marked this pull request as ready for review November 5, 2024 20:14
Copy link
Collaborator

@atrigila atrigila left a comment

Choose a reason for hiding this comment

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

  • Small suggestions to document the reason for the changes in the PR.
  • Add information to changelog.
  • Test (and post results) of the full chromosome test before the full test.

conf/test_full.config Outdated Show resolved Hide resolved
modules/local/bam_chr_extract/main.nf Outdated Show resolved Hide resolved
modules/local/vcf_chr_extract/main.nf Outdated Show resolved Hide resolved
subworkflows/local/bam_chr_rename_samtools/main.nf Outdated Show resolved Hide resolved
subworkflows/local/bam_impute_quilt/main.nf Outdated Show resolved Hide resolved
subworkflows/local/utils_nfcore_chrcheck_pipeline/main.nf Outdated Show resolved Hide resolved
conf/test_full.config Outdated Show resolved Hide resolved
conf/test_full.config Outdated Show resolved Hide resolved
@atrigila atrigila changed the title Fulltest Reduce computational time of AWS megatest and language server fixes Nov 6, 2024
workflows/phaseimpute/tests/main.nf.test.snap Show resolved Hide resolved
conf/test_all.config Outdated Show resolved Hide resolved
Copy link
Collaborator

@atrigila atrigila left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. There’s a lot of great work in here. I noticed, though, that the scope of this PR is still a bit broad. Since the main focus is on "reducing computational time for the AWS megatest," could we keep this PR to just those specific changes? Some of the updates seem unrelated, and reviewing all 64 files at once makes it a bit tough to keep things focused.

Would you mind splitting out the unrelated changes into separate PRs (e.g. subworkflow snapshots, major language server fixes)? I'll make it better for me to give each code the attention it deserves. Also, include the results of at least one full chromosome test (you can run it on a large gitpod instance with a small sample and it shouldn't take that long) showing the performance improvement will be super helpful to speed up the review.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
workflows/phaseimpute/tests/main.nf.test.snap Show resolved Hide resolved
@LouisLeNezet LouisLeNezet changed the title Reduce computational time of AWS megatest and language server fixes Update nf-tests and language server fixes Nov 10, 2024
@LouisLeNezet
Copy link
Collaborator Author

The scope of the PR has been changed to only fix the language server and nf-test updates.
The fulltest changes has been moved to #158

@LouisLeNezet LouisLeNezet merged commit 10a1dda into nf-core:dev Nov 11, 2024
12 checks passed
@LouisLeNezet LouisLeNezet linked an issue Nov 13, 2024 that may be closed by this pull request
@LouisLeNezet LouisLeNezet deleted the fulltest branch November 18, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update all nf-test
2 participants