Resolve CLI test suite IDs against the live dropdown#620
Merged
tracygardner merged 2 commits intoflipcomputing:mainfrom May 8, 2026
Merged
Resolve CLI test suite IDs against the live dropdown#620tracygardner merged 2 commits intoflipcomputing:mainfrom
tracygardner merged 2 commits intoflipcomputing:mainfrom
Conversation
Running `npm run test:api @<suite>` for any suite whose dropdown ID is
the bare name (e.g. `@sensing`, `@materials`, `@physics`) failed with a
30s Playwright timeout and a misleading "did not find some options"
message. Three suites in tests/tests.html use `@`-prefixed dropdown IDs
(`@new`, `@notslow`, `@onlyslow`), but for ~20 others the `@`-prefix is
the mocha grep pattern, not the dropdown ID. The runner passed the
user's argument straight to `page.selectOption`, so either form had to
match the dropdown exactly.
Replace the static AVAILABLE_SUITES lookup with `resolveSuite(id,
dropdownIds)` that:
- Reads the actual dropdown options after the page loads.
- Accepts either `@<id>` or `<id>` and resolves to whichever matches.
- Polls for up to 30s, matching Playwright's default selectOption
timeout, because tests.html's loadAllTests populates the dropdown
incrementally (each option is appended before its module is awaited).
- On miss, throws immediately with the full list of valid IDs and a
hint about the `@`/grep-pattern distinction, replacing the opaque
"did not find some options" timeout.
Also remove the `movement: "translation"` alias. `movement` and
`translation` are separate suites with separate test files
(movement.test.js vs transform.translate.test.js); the alias caused
`npm run test:api movement` to silently run translation tests instead
of the 10 movement tests.
Fixes flipcomputing#619
Co-Authored-By: Claude Opus 4.7 <[email protected]>
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDelays mapping the CLI suite argument until after the test page's dropdown is populated: adds resolveSuite to accept ChangesSuite Resolution and Dropdown Polling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/run-api-tests.mjs (1)
163-171: ⚡ Quick win
resolveSuiteonly resolves@foo → foo, not the reversefoo →@foo``.The function handles the primary PR use-case (
@sensing→sensing) but has no path for the inverse: a user who typesnotslow,new, oronlyslow(without@) will hit the 30-second poll and receive a confusing "not in the dropdown" error, because the three special dropdown entries are@notslow,@onlyslow,@new.♻️ Proposed fix — add the `bare → `@bare`` fallback
function resolveSuite(id, dropdownIds) { const aliased = suiteAliases[id] || id; if (dropdownIds.includes(aliased)) return aliased; if (aliased.startsWith("@")) { const bare = aliased.slice(1); if (dropdownIds.includes(bare)) return bare; - } + } else { + const withAt = "@" + aliased; + if (dropdownIds.includes(withAt)) return withAt; + } return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run-api-tests.mjs` around lines 163 - 171, The resolveSuite function currently only maps aliased IDs beginning with "@" to their bare forms but doesn't handle the inverse (e.g., user types "notslow" while dropdown contains "@notslow"); update resolveSuite to check both the aliased and its "@"-prefixed counterpart: after computing aliased = suiteAliases[id] || id, first return aliased if dropdownIds.includes(aliased), then if dropdownIds.includes("@" + aliased) return "@" + aliased; keep the existing branch that strips "@" when aliased startsWith("@") and returns the bare value if present; ensure the function still returns null when no match is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/run-api-tests.mjs`:
- Around line 163-171: The resolveSuite function currently only maps aliased IDs
beginning with "@" to their bare forms but doesn't handle the inverse (e.g.,
user types "notslow" while dropdown contains "@notslow"); update resolveSuite to
check both the aliased and its "@"-prefixed counterpart: after computing aliased
= suiteAliases[id] || id, first return aliased if dropdownIds.includes(aliased),
then if dropdownIds.includes("@" + aliased) return "@" + aliased; keep the
existing branch that strips "@" when aliased startsWith("@") and returns the
bare value if present; ensure the function still returns null when no match is
found.
CodeRabbit noted: ``` resolveSuite only resolves @foo → foo, not the reverse foo → @foo``. The function handles the primary PR use-case (@SenSing → sensing) but has no path for the inverse: a user who types notslow, new, or onlyslow (without @) will hit the 30-second poll and receive a confusing "not in the dropdown" error, because the three special dropdown entries are @notslow, @onlyslow, @new. ``` I've added the code it suggested and tested the resolution in both directions - they worked.
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
2 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.
Running
npm run test:api @<suite>for any suite whose dropdown ID is the bare name (e.g.@sensing,@materials,@physics) failed with a 30s Playwright timeout and a misleading "did not find some options" message. Three suites in tests/tests.html use@-prefixed dropdown IDs (@new,@notslow,@onlyslow), but for ~20 others the@-prefix is the mocha grep pattern, not the dropdown ID. The runner passed the user's argument straight topage.selectOption, so either form had to match the dropdown exactly.Replace the static AVAILABLE_SUITES lookup with
resolveSuite(id, dropdownIds)that:@<id>or<id>and resolves to whichever matches.@/grep-pattern distinction, replacing the opaque "did not find some options" timeout.Also remove the
movement: "translation"alias.movementandtranslationare separate suites with separate test files (movement.test.js vs transform.translate.test.js); the alias causednpm run test:api movementto silently run translation tests instead of the 10 movement tests.Fixes #619
Summary by CodeRabbit