Set up Cram tests, fix fetch-from-ncbi-virus#17
Merged
Conversation
victorlin
force-pushed
the
victorlin/cram-tests-and-fixes
branch
from
d6cb670 to
7e56655
Compare
victorlin
commented
Aug 30, 2023
2 tasks
We definitely don't need "the system bash", which is what /bin/bash is. We want "the bash the user wants", which will often be the system bash, but not always. The main motivation is that /bin/bash on macOS is stuck on an ancient version due to licensing issues, and users might prefer another Bash in their environments.
joverlee521
reviewed
Aug 31, 2023
victorlin
force-pushed
the
victorlin/cram-tests-and-fixes
branch
from
7e56655 to
34089c4
Compare
joverlee521
approved these changes
Aug 31, 2023
Contributor
joverlee521
left a comment
joverlee521
left a comment
There was a problem hiding this comment.
Thanks for adding the Cram tests!
| $ $TESTDIR/../../fetch-from-ncbi-virus 12637 nextstrain/ingest \ | ||
| > --filters 'CreateDate_dt:([1987-11-29T00:00:00Z TO 1987-11-29T00:00:01Z])' \ | ||
| > --fields 'viruslineage_ids:VirusLineageId_ss' | ||
| {"genbank_accession":"X05375","genbank_accession_rev":"X05375.1","database":"GenBank","strain":"","region":"","location":"","collected":"","submitted":"1987-11-29T00:00:00Z","updated":"2016-07-26T00:00:00Z","length":"360","host":"","isolation_source":"","bioproject_accession":"","biosample_accession":"","sra_accession":"","title":"Dengue virus type 2 genomic RNA for envelope protein E N-term","authors":"Biedrzycka,A., Cauchi,M.R., Bartholomeusz,A., Gorman,J.J., Wright,P.J.","submitting_organization":"","publications":"2952760","sequence":"GTAACTTATGGGACGTGTACCACCACAGGAGAACACAGAAGAGAAAAAAGATCAGTGGCACTCGTTCCACATGTGGGAATGGGACTGGAGACACGAACTGAAACATGGATGTCATCAGAAGGGGCCTGGAAACATGCCCAGAGAATTGAAACTTGGATCTTGAGACATCCAGGCTTTACCATAATGGCAGCAATCCTGGCATACACCATAGGAACGACACATTTCCAAAGAGCCCTGATTTTCATCTTACTGACAGCTGTCGCTCCTTCAATGACAATGCGTTGCATAGGAATATCAAATAGAGACTTTGTAGAAGGGGTTTCAGGAGGAAGCTGGGTTGACATAGTCTTAGAACATGGA","viruslineage_ids":"10239,2559587,2732396,2732406,2732462,2732545,11050,11051,12637,11060"} |
Contributor
There was a problem hiding this comment.
Not sure how we can guard against these values changing in the future, but hopefully that won't happen any time soon since this was last updated in 2016!
Member
Author
There was a problem hiding this comment.
Something like jq to check only a few JSON keys would be ideal, but I don't want to add another dev dependency right now.
victorlin
force-pushed
the
victorlin/cram-tests-and-fixes
branch
from
34089c4 to
993f3ab
Compare
Simplify the bash script by directly passing these options to the script that uses them. This requires changing the order so that required arguments are specified before options. This also removes reliance on the Bash ≥4 feature that allows unset arrays to be accessed by [@] with -u.
victorlin
force-pushed
the
victorlin/cram-tests-and-fixes
branch
from
993f3ab to
966ebcf
Compare
tsibley
added a commit
to nextstrain/nextstrain.org
that referenced
this pull request
May 13, 2024
@victorlin noted in review that the previous construct threw an "unbound variable" error on macOS's system Bash 3.2.57.¹ This is because after 3.2.57 and at least by 4.4.20 (but maybe earlier), Bash stopped treating the "@" and "*" parameters as unset under -u when empty.² Use an array slice³ (i.e. substring expansion, ${parameter:offset}, applied to an array) as a workaround. Note that testing if "docker_args" is empty, e.g. "${docker_args:+${docker_args[@]}}", won't work because it leaves us with an empty string argument when unset. ¹ <#857 (comment)> ² <nextstrain/shared#17 (comment)> ³ <https://www.gnu.org/software/bash/manual/bash.html#Shell-Parameter-Expansion>
j23414
pushed a commit
to nextstrain/nextstrain.org
that referenced
this pull request
Mar 11, 2025
@victorlin noted in review that the previous construct threw an "unbound variable" error on macOS's system Bash 3.2.57.¹ This is because after 3.2.57 and at least by 4.4.20 (but maybe earlier), Bash stopped treating the "@" and "*" parameters as unset under -u when empty.² Use an array slice³ (i.e. substring expansion, ${parameter:offset}, applied to an array) as a workaround. Note that testing if "docker_args" is empty, e.g. "${docker_args:+${docker_args[@]}}", won't work because it leaves us with an empty string argument when unset. ¹ <#857 (comment)> ² <nextstrain/shared#17 (comment)> ³ <https://www.gnu.org/software/bash/manual/bash.html#Shell-Parameter-Expansion>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
Currently there's a gap in testing the scripts in this repo. Maybe Cram can help (partially) fill that gap. At the least, I've provided an example showing how it can be useful for
fetch-from-ncbi-virus.Related issue(s)
Fixing bugs surfaced by nextstrain/mpox#175
Checklist
If adding a script, add an entry for it in the README.fetch-from-ncbi-virusto allow skipping--filterand--fieldon older Bash versions