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

Updated to support changes in mg-scripts for multiple config. #78

Merged
merged 14 commits into from
Feb 16, 2024

Conversation

charles-cowart
Copy link
Contributor

qp-klp configuration-related code/unittests updated to support multiple configurations and the latest changes in mg-scripts.
There is a single test (test_replicates) that needs some additional updates.

@@ -0,0 +1,91 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this and the other json here? I thought they could be taken from mg-scripts ... no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. these are to support the unittests, which require initializing a pipeline object and hence a sample configuration file and configuration_profiles directory with sample profiles.

@@ -0,0 +1,265 @@
#!/bin/bash -l
Copy link
Member

Choose a reason for hiding this comment

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

Why is this file now needed in the tests vs. just leaving it in the templates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's much less time-consuming to simply load the expected value from a file and compare it with the observed results rather than keep trying to make point changes to a list of strings that are often split to placate flake8. We really do modify that batch script often, at least currently.

Copy link
Member

Choose a reason for hiding this comment

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

however, I thought we were not adding tests that compared this file in a tests ... no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're not adding new tests but this was an existing test. I could replace the test for one that simply checks for the existence of a job-script in the output and maybe checks the first few lines, but that seems a step backwards.

@charles-cowart charles-cowart changed the title WIP: Updated to support changes in mg-scripts for multiple config. Updated to support changes in mg-scripts for multiple config. Feb 15, 2024
@charles-cowart
Copy link
Contributor Author

PR is ostensibly done. Reference to mg-scripts needs to point back to master once the latest mg-scripts PR has been reviewed, approved, and merged. Latest push didn't trigger CI, but includes the fixes to resolve all outstanding issues observed in CI.

@antgonza antgonza merged commit bf0d6c9 into qiita-spots:main Feb 16, 2024
2 checks passed
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.

2 participants