Fix #104, #105, #107: CWL duplicate, PAE coverage, env restore - #113
Merged
Conversation
test_docker_image_for_file_uri_raises set the env var by hand and did `del os.environ[...]` in its finally block, which erases a value that was already set before the run (a developer pointing at a custom tools.yml) and leaves every later test resolving a different config than it started with. monkeypatch.setenv restores the prior state automatically, including the was-not-set case. The _load_config.cache_clear() calls stay -- the loader is lru_cached, so without them the temp config leaks into subsequent tests. No other test in the repo manipulates os.environ directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DEWo4xvL59PHC1QUvtTV9F
…-report.cwl Both files hashed to 3ef5561ea1273e75546df5827269f95e, so selecting the "-msa" workflow for MSA behavior silently ran the plain one. Chose deletion over implementing a real MSA variant: boltz-report.cwl already declares `use_msa_server` (default true) and wires it into the predict step, which is exactly what the name promised -- MSA here is a job parameter, not a different workflow topology. A file-based MSA input is likewise already exposed by protein-structure-prediction.cwl and multi-tool-comparison.cwl via their `msa` input. Forking a workflow per parameter value is not a pattern used anywhere else in cwl/ (there is no chai-report-msa.cwl, no esmfold-report-msa.cwl), so a second copy would only re-create the drift risk. References repointed: crambin-boltz-msa-report.yml now documents itself as a boltz-report.cwl job with use_msa_server explicitly on (its inputs were already valid for that workflow), and the PAE-wiring test drops the deleted file from its parametrize list. docs/cwl-test-report-260329.2.md is a dated test report and is left as the historical record it is. Regression guard: TestNoDuplicateCWLDefinitions hashes every cwl/**/*.cwl and fails on any byte-identical pair, so a copied-and-renamed workflow cannot come back quietly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DEWo4xvL59PHC1QUvtTV9F
select-pae.cwl was covered by `cwltool --validate` plus a YAML structure check, neither of which runs the JavaScript expression -- a typo in the basename match or a wrong listing walk passed CI and would only surface in a production CWL run. TestSelectPaeExecution runs the tool for real against tmp_path fixtures holding a couple of one-line files: pae.json under predictions/ (the normalized layout), pae.json at the top level (raw tool output), and a directory with no PAE at all to exercise the optional-output path. It asserts on the contents of the selected file, not just its presence. Verified by mutation: breaking the basename comparison in the expression fails two of the three new tests while the existing --validate test still passes. Skips when cwltool is not on PATH, matching the shutil.which idiom in tests/acceptance/test_cwl_workflow_execution.py. Each case takes ~1.5s; the generous per-class timeout only covers slow cwltool startup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DEWo4xvL59PHC1QUvtTV9F
Its D5 case and two recommendations point at boltz-report-msa.cwl, which #104 removed; a reader following them would chase a missing file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DEWo4xvL59PHC1QUvtTV9F
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
1 participant
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.
Closes #104. Closes #105. Closes #107.
Three independent commits, reviewable separately.
#107 —
tests/test_config.pydeletedPREDICT_STRUCTURE_CONFIGinfinallyinstead of restoring it, erasing a pre-existing setting. Nowmonkeypatch.setenv; the_load_config.cache_clear()calls stay (the loader islru_cached). Audited the repo — this was the only occurrence.#104 —
boltz-report-msa.cwlwas byte-identical toboltz-report.cwl, promising MSA behavior it did not carry. Deleted rather than reimplemented:boltz-report.cwlalready declaresuse_msa_server(boolean?, default true) and wires it into the predict step, so MSA here is a job-file parameter, not a different workflow topology — and forking a workflow per parameter value is a pattern used nowhere else incwl/. The job file's header comment was repointed, the test parametrize entry dropped, and the dated March CWL report is now flagged as historical so its D5 references don't send readers after a deleted file. A new test md5-hashes everycwl/**/*.cwland fails on any byte-identical pair, so this cannot silently recur.#105 —
select-pae.cwlhad validate-only coverage. New tests execute it viacwltoolfor three cases:pae.jsonunderpredictions/, at top level, and absent (optional-output path). These ran rather than skipped — cwltool is on PATH — and were mutation-checked by breaking the basename comparison, which the pre-existing--validatetests did not catch.594 passed, 10 skipped.