fix(types): allow Promise<boolean> as async test return#1254
fix(types): allow Promise<boolean> as async test return#1254rohan-devbuilds wants to merge 1 commit intoealush:latestfrom
Conversation
|
@rohan-devbuilds is attempting to deploy a commit to the ealush's projects Team on Vercel. A member of the Team first needs to authorize it. |
Review Summary by QodoAllow Promise<boolean> as async test return type
WalkthroughsDescription• Allow Promise<boolean> return type in async tests • Update AsyncTest type definition to accept both void and boolean • Aligns TypeScript types with documented vest patterns Diagramflowchart LR
A["AsyncTest Type"] -- "changed from" --> B["Promise<void>"]
A -- "changed to" --> C["Promise<void | boolean>"]
C -- "enables" --> D["skipWhen async tests"]
File Changes1. packages/vest/src/core/test/TestTypes.ts
|
Code Review by Qodo
1. Async boolean not evaluated
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a type incompatibility issue in Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughA single type definition is expanded to allow async test functions to optionally return boolean values in addition to void, enabling test callbacks to indicate conditions without throwing errors. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the AsyncTest type definition in packages/vest/src/core/test/TestTypes.ts. The change from Promise<void> to Promise<void | boolean> allows asynchronous tests to return a promise that resolves to a boolean value. This aligns the type system with existing functionality, resolving a type error for a documented usage pattern.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| export type TestFn = (payload: TestFnPayload) => TestResult; | ||
| export type AsyncTest = Promise<void>; | ||
| export type AsyncTest = Promise<void | boolean>; |
There was a problem hiding this comment.
1. Async boolean not evaluated 🐞 Bug ✓ Correctness
Changing AsyncTest to Promise<void | boolean> allows async test functions to resolve false, but the async execution path ignores the resolved value and marks the test completed/passing unless the promise rejects. This can silently turn async () => false into a passing validation.
Agent Prompt
## Issue description
`AsyncTest` was widened to `Promise<void | boolean>`, but the async test runner ignores the promise’s resolved value and only fails on rejection. As a result, `async () => false` will incorrectly pass.
## Issue Context
Synchronous tests explicitly fail when returning `false`, while async tests call `onTestCompleted` on fulfillment without inspecting the fulfillment value.
## Fix Focus Areas
- packages/vest/src/core/test/testLevelFlowControl/runTest.ts[120-151]
- packages/vest/src/core/test/__tests__/runAsyncTest.test.ts[135-189]
## Suggested approach
1. Change the async fulfillment handler to accept the resolved value (e.g., `(value?: void | boolean) => { ... }`).
2. If the resolved value is exactly `false`, call `VestTest.fail(testObject)` before completing.
3. Keep rejection handling as-is.
4. Add tests:
- An async test that `return false;` and verify final status is failing.
- An async test that `return true;` (or returns void) and verify passing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vest/src/core/test/TestTypes.ts`:
- Line 8: The async test handler currently discards the resolved Promise value;
update the persisted done callback created via VestRuntime.persist in runTest.ts
so it accepts the resolved result (result?: void | boolean), checks if result
=== false and calls VestTest.fail(testObject) in that case, then calls
onTestCompleted(testObject).unwrap(); this ensures asyncTest.then(done, fail)
propagates a resolved false as a failure just like sync tests while keeping the
existing failure path intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f128beb-7c7e-4190-be19-ce7af9a9a3e3
📒 Files selected for processing (1)
packages/vest/src/core/test/TestTypes.ts
|
|
||
| export type TestFn = (payload: TestFnPayload) => TestResult; | ||
| export type AsyncTest = Promise<void>; | ||
| export type AsyncTest = Promise<void | boolean>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if there are any tests or documentation about async tests returning false
rg -n -C3 'async.*return.*false' --type=ts
rg -n 'Promise.*false' --type=tsRepository: ealush/vest
Length of output: 358
🏁 Script executed:
#!/bin/bash
# Read runTest.ts to see the actual implementation around lines 114-151
cat -n packages/vest/src/core/test/testLevelFlowControl/runTest.ts | sed -n '100,160p'Repository: ealush/vest
Length of output: 2132
🏁 Script executed:
#!/bin/bash
# Search for onTestCompleted and related implementations
rg -n 'onTestCompleted' --type=ts -A5 | head -80
rg -n 'VestTest.pass' --type=ts -B2 -A2 | head -80Repository: ealush/vest
Length of output: 5463
🏁 Script executed:
#!/bin/bash
# Look for test files that test async test behavior
fd 'test|spec' --type=f --path='*packages/vest*' | head -20
# Also search for skipWhen patterns with async tests
rg -n 'skipWhen' --type=ts -B2 -A2 | head -60Repository: ealush/vest
Length of output: 4064
🏁 Script executed:
#!/bin/bash
# Find test files for async test handling
find packages/vest -type f -name "*.test.ts" -o -name "*.spec.ts" | head -20
rg -n 'async.*test' packages/vest/src --type=ts -B2 -A5 | grep -E "(async|Promise|\.then)" | head -40Repository: ealush/vest
Length of output: 6174
🏁 Script executed:
#!/bin/bash
# Check skipWhen implementation
cat packages/vest/src/isolates/skipWhen.tsRepository: ealush/vest
Length of output: 1511
🏁 Script executed:
#!/bin/bash
# Look for any comments or references to boolean returns in async tests
rg -n 'Promise.*boolean\|boolean.*Promise\|async.*return.*false' --type=ts -B3 -A3 | head -60Repository: ealush/vest
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Look at the async tests integration file
cat packages/vest/src/__tests__/integration.async-tests.test.ts | head -200Repository: ealush/vest
Length of output: 2809
🏁 Script executed:
#!/bin/bash
# Check the synchronous test handling in runTest.ts
cat -n packages/vest/src/core/test/testLevelFlowControl/runTest.ts | sed -n '40,110p'Repository: ealush/vest
Length of output: 2595
🏁 Script executed:
#!/bin/bash
# Search for skipWhen with async tests that might return boolean
rg -n 'skipWhen' packages/vest/src/__tests__ --type=ts -A10 | grep -E '(skipWhen|async|Promise|return)' | head -40Repository: ealush/vest
Length of output: 1264
🏁 Script executed:
#!/bin/bash
# Look for tests that might test returning boolean from async tests
rg -n 'Promise.*false\|Promise.*true\|Promise\.resolve.*false\|Promise\.resolve.*true' packages/vest/src --type=tsRepository: ealush/vest
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check the TestTypes file and surrounding context
cat -n packages/vest/src/core/test/TestTypes.tsRepository: ealush/vest
Length of output: 557
Async test handler ignores the resolved boolean value.
The type broadening to Promise<void | boolean> correctly addresses the TypeScript issue, but the async test handler in packages/vest/src/core/test/testLevelFlowControl/runTest.ts (lines 128-130) discards the resolved value:
const done = VestRuntime.persist(() => {
onTestCompleted(testObject).unwrap();
});The done callback accepts no parameters, so asyncTest.then(done, fail) at line 150 ignores the promise's resolved value entirely. This creates an inconsistency:
- Sync test:
return false→ test fails ✓ - Async test:
return Promise.resolve(false)→ test passes ✗
The done callback must be updated to receive and check the resolved value:
const done = VestRuntime.persist((result?: void | boolean) => {
if (result === false) {
VestTest.fail(testObject);
}
onTestCompleted(testObject).unwrap();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vest/src/core/test/TestTypes.ts` at line 8, The async test handler
currently discards the resolved Promise value; update the persisted done
callback created via VestRuntime.persist in runTest.ts so it accepts the
resolved result (result?: void | boolean), checks if result === false and calls
VestTest.fail(testObject) in that case, then calls
onTestCompleted(testObject).unwrap(); this ensures asyncTest.then(done, fail)
propagates a resolved false as a failure just like sync tests while keeping the
existing failure path intact.
1 participant
The AsyncTest type was defined as Promise, which caused TypeScript to reject async test functions that return Promise inside skipWhen. This is a documented and valid pattern in vest's own docs.
Changed AsyncTest from Promise to Promise<void | boolean> so TypeScript accepts both return types without errors.
Fixes #1156
Summary by CodeRabbit