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

Fix for converter tests #17188

Merged

Conversation

bernt-matthias
Copy link
Contributor

First I tried to be as unintrusive as possible, i.e. to keep assertions for the zipped output. But finally I went for having only tests wrt the decompressed output (i.e. with decompress="true").

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 23.2 milestone Dec 14, 2023
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@mvdbeek
Copy link
Member

mvdbeek commented Dec 14, 2023

Can you also fix the vcf_to_vcf_bgzip test ? And do we know why the test outputs are suddenly larger ?

@bernt-matthias
Copy link
Contributor Author

Can you also fix the vcf_to_vcf_bgzip test ?

Sure

And do we know why the test outputs are suddenly larger ?

I'm also wondering. I thought that it's a conincidence of #13411 and
#15085 .. but this is to long ago I guess.

Since we use containerized tests and the containers were not updated recently (in the last months) also this can't be the reason.

Did we change something in the testing recently?

@mvdbeek
Copy link
Member

mvdbeek commented Dec 14, 2023

Can you please make sure the converters run and that the output is compressed ?

@bernt-matthias
Copy link
Contributor Author

bernt-matthias commented Dec 14, 2023

Ahh... the converter tests use planemo .. so they use the available planemo and the available packages .. and the planemo supporting 23.1 was just release on December 6.

Edit: so its #15085 .. delayed by the release of planemo.

@bernt-matthias
Copy link
Contributor Author

Checked my local planemo test for the vcf_to_vcf_bgzip_converter. The test ran exactly two tools: upload and the converter and the output file is Blocked GNU Zip Format (BGZF; gzip compatible), block length 688

@mvdbeek
Copy link
Member

mvdbeek commented Dec 14, 2023

Can we instead assert the size against the API value for file_size ? I don't know if comparing against the decompressed size is really all that intuitive ? I don't like the idea of "breaking" tests this way.

@bernt-matthias
Copy link
Contributor Author

Can we instead assert the size against the API value for file_size ?

Sure possible. We can even do both (as I did in the first commit).

But I would suggest to add a delta, since in my experience zip files tend to have different file sizes over time (probably because the compression algorithm changes). Do you have a suggestion? 10% of the size?

@mvdbeek
Copy link
Member

mvdbeek commented Dec 15, 2023

ok, I guess we just gotta live with this.

@mvdbeek mvdbeek merged commit 2891d70 into galaxyproject:dev Dec 15, 2023
46 checks passed
@bernt-matthias bernt-matthias deleted the fix/interval_bgzip_converter branch December 15, 2023 12:17
@bernt-matthias
Copy link
Contributor Author

ok, I guess we just gotta live with this.

Not necessarily, was just busy with the linters PR.

How about testing with <has_size max="DECOMPRESSED_SIZE-1"/>` .. would be a very basic test for a compression (which would fail only for a single test of these tools where the decompressed file size is very small).

@mvdbeek
Copy link
Member

mvdbeek commented Dec 15, 2023

I don't care about the tests here, I care about breaking tool tests we don't control.

@mvdbeek mvdbeek modified the milestones: 23.2, 24.0 Dec 19, 2023
bgruening added a commit that referenced this pull request Dec 23, 2023
[23.1] Backport #17188: Fix for converter tests
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.

2 participants