test(workflows): name the condition-rejection tests for the real boundary - #3808
Open
jawwad-ali wants to merge 1 commit into
Open
test(workflows): name the condition-rejection tests for the real boundary#3808jawwad-ali wants to merge 1 commit into
jawwad-ali wants to merge 1 commit into
Conversation
…dary `test_validate_rejects_non_string_condition` contradicts its sibling `test_validate_accepts_string_or_bool_condition` in the same class: a bool *is* a non-string, so the two names disagree about the contract the validator actually implements. Rename to `test_validate_rejects_non_string_non_bool_condition` in all three step classes, matching the validator's own message: "'condition' must be a string or boolean, got <type>". Test names only — no behaviour change, and the parametrized values are untouched. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Renames three workflow validation tests to accurately describe the accepted string-or-boolean boundary.
Changes:
- Clarifies rejection test names for if, while, and do-while steps.
- Makes no behavioral changes.
Show a summary per file
| File | Description |
|---|---|
tests/test_workflows.py |
Corrects three condition-validation test names. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
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.
Small follow-up to #3706 (merged in a9c9905), addressing the last Copilot note on it. Test names only — no behaviour change.
Problem
#3706 landed a pair of tests in each of the three conditional step classes, and their names contradict each other:
A bool is a non-string, so the first name claims something the second one disproves. Copilot flagged exactly this on #3706 (three suppressed low-confidence notes on
tests/test_workflows.py), but the review landed close to the merge, so it wasn't picked up:That's my leftover: when I updated the contract to accept booleans I renamed the acceptance test and missed its sibling.
Fix
in
TestIfThenStep,TestWhileStepandTestDoWhileStep— matching the validator's own message,'condition' must be a string or boolean, got <type>.3 insertions, 3 deletions, one file. Parametrized values untouched; no source changes.
Why it's worth a PR
The names appear in every test report and in
-kselections, sorejects_non_stringreads as a contract the code deliberately does not implement — acceptingboolis intentional (condition: falseresolves correctly today, and two of the three steps defaultconditionto the boolFalsethemselves, so rejecting it would be a breaking change).Verification
uvx [email protected] check src tests→ cleanmain(a9c9905)Written with assistance from Claude Code.