Derive the test symbol from the registry instead of naming VIX - #20
Merged
Conversation
main is red. #18 and #19 are each correct and green on their own base, and break when combined: #18's tests used VIX as the example of a symbol that IS registered, and #19 retired VIX from registry.yaml. Neither PR's CI could see it -- #18 was branched before #19 landed, so the two never ran against each other until the second merge produced a commit no pipeline had tested. The fixture now asks the registry for an equities symbol rather than naming one. There is no list to keep in step, so the next retirement cannot turn these tests red for a reason that has nothing to do with what they assert. Worth noting what did NOT go wrong: the guard behaved exactly as designed. It refused `--symbols VIX` against a registry that no longer carries VIX, which is the whole point of #18, and #19's commit message predicted that consequence in advance. The tests asserted the old world, not the guard. ruff clean; `pytest tests/ -q -m "not network"` (CI's invocation) 205 passed, 12 deselected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
mainis red at80b60df. This fixes it.What happened
#18 and #19 are each correct, and each was green on its own base. They break when combined:
tests/test_cli_symbols.py, which usedVIXas its example of a symbol that is in the registry.VIXfromregistry.yaml.#18 branched before #19 landed, so the two never ran against each other. The merge produced a commit no pipeline had tested, and two cases went red:
What did not go wrong
The guard did exactly what #18 built it to do — refuse
--symbols VIXagainst a registry that no longer carries VIX — and #19's own commit message predicted that consequence in writing. The production code is right in both PRs. Only the test fixture asserted a world that no longer exists.The fix
The fixture reads an equities symbol out of the registry instead of naming one:
There is no longer a symbol name to keep in step, so a future retirement cannot turn these tests red for a reason unrelated to what they assert. That is the same class of drift the module was written to catch, so it should not have been reproduced in its own fixture.
Verification
ruff check src testsclean.pytest tests/ -q -m "not network"— CI's exact invocation — 205 passed, 12 deselected.🤖 Generated with Claude Code