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

Adding MIP validation cases to fastq.py #771

Merged
merged 16 commits into from Sep 29, 2020
Merged

Conversation

henningonsbring
Copy link
Contributor

@henningonsbring henningonsbring commented Sep 23, 2020

This PR adds cases that should not be compressed since these families are often used for validation.

How to prepare for test

  • wait for the automatic fastq compression to start

Expected test outcome

  • Check that the problematic/validation cases where skipped
  • Take a screenshot and attach or copy/paste the output.

Review

  • code approved by @moonso
  • tests executed by
  • "Merge and deploy" approved by @moonso
    Thanks for filling in who performed the code review and the test!

This version is a

  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

@coveralls
Copy link
Collaborator

coveralls commented Sep 23, 2020

Coverage Status

Coverage increased (+0.003%) to 71.519% when pulling 939b0e4 on henningonsbring-patch-1 into 30775ea on master.

@Mropat
Copy link
Contributor

Mropat commented Sep 23, 2020

Could we also put keenviper (mip-dna) and sharpparrot (mip-rna) in the cases to ignore list?

Maybe make their own list of validation samples, and unpack both validation list and problematic cases list when skipping

Copy link
Contributor

@moonso moonso left a comment

Choose a reason for hiding this comment

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

Nice ⭐

@Mropat
Copy link
Contributor

Mropat commented Sep 23, 2020

Are we sure the only way to test this is to deploy? There was no test for skipping cases in compress?

Copy link
Contributor

@Mropat Mropat left a comment

Choose a reason for hiding this comment

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

You can create tests for this by:

in tests/cli/compress/ you could do either:
1.

  • Add problematic families to populated compress store in tests/cli/compress/conftest.py
  • Write test for fastq_cmd checking the log for skipped case message
  • Write test for fastq_cmd while adding the problematic cases to ready-made populated_compress_store, checking the log for skipped case message

@hassanfa
Copy link
Contributor

Could we add the following to the list as well?

moralgoat - wgs paired
fleetjay - wgs single
bosssponge - panel single
unitedbeagle - panel paired

I test run on these when testing BALSAMIC releases.

Thanks!

Copy link
Contributor

@Mropat Mropat left a comment

Choose a reason for hiding this comment

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

It would be great to add a test for this, but since this might be not the n1 prio right now, we could deploy this as is

@moonso
Copy link
Contributor

moonso commented Sep 28, 2020

@henningonsbring is this ready?

@henningonsbring
Copy link
Contributor Author

No, there is something I need to solve in production before I deal with this

@sonarcloud
Copy link

sonarcloud bot commented Sep 29, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Mropat Mropat merged commit e6c7726 into master Sep 29, 2020
@Mropat Mropat deleted the henningonsbring-patch-1 branch September 29, 2020 14:17
@Mropat
Copy link
Contributor

Mropat commented Sep 29, 2020

Deployed hasta-stage
image

Deployed on hasta-prod
image

@Mropat
Copy link
Contributor

Mropat commented Sep 29, 2020

Congratulations @henningonsbring on your first contribution to cg! 🥳

@moonso
Copy link
Contributor

moonso commented Sep 29, 2020

Nice work @henningonsbring !

@henningonsbring
Copy link
Contributor Author

Thank you for the help! :)

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

5 participants