fix(tests): use this.skip() when cross-window popup is blocked#356
fix(tests): use this.skip() when cross-window popup is blocked#356brianmhunt merged 2 commits intomainfrom
Conversation
Both cross-window specs silently `return` when `window.open`
yields null (popup blocker). Mocha counts a sync function that
throws nothing as a pass, so blocked-popup runs looked green
while actually skipping every cross-window assertion — a silent
coverage gap.
Swap the bare `return` for `return this.skip('popup blocked — ...')`
so the suite correctly reports these as pending (○) in the
stats chip, making the blocked state visible to contributors
running /tests and to CI.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 32 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo cross-window behavior test files updated to explicitly skip with a message when Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Pull request overview
Makes cross-window test behavior visible when browsers block popups, preventing silent “passes” that actually skipped all assertions.
Changes:
- Replace silent early
returnwiththis.skip('popup blocked — cross-window assertions cannot run')in the cross-window specs. - Ensures blocked-popup runs show up as pending in Mocha output rather than passing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/bind/spec/crossWindowBehaviors.ts | Marks the test as pending when window.open returns null (popup blocked). |
| builds/knockout/spec/crossWindowBehaviors.js | Same pending-on-popup-blocked behavior for the knockout build spec. |
![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/bind/spec/crossWindowBehaviors.ts`:
- Line 53: The test callback currently calls this.skip('popup blocked —
cross-window assertions cannot run') but must accept the ctx: any parameter used
across this codebase; change the test function signature to include ctx (e.g.,
(ctx: any) => { ... }) and replace this.skip(...) with ctx.skip('popup blocked —
cross-window assertions cannot run') so the wrapped skip supporting a message is
used (follow the pattern in packages/utils/helpers/test-env.ts and other
environment-conditional skips).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e6d2f2b-5910-4d85-98f6-715bac97be48
📒 Files selected for processing (2)
builds/knockout/spec/crossWindowBehaviors.jspackages/bind/spec/crossWindowBehaviors.ts
|
|
||
| if (!win2) { | ||
| return | ||
| return this.skip('popup blocked — cross-window assertions cannot run') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the content around line 53
find . -type f -name "crossWindowBehaviors.ts" | head -5Repository: knockout/tko
Length of output: 100
🏁 Script executed:
# Look at the file content around line 53
cat -n packages/bind/spec/crossWindowBehaviors.ts | sed -n '45,65p'Repository: knockout/tko
Length of output: 1015
🏁 Script executed:
# Check if TypeScript config and test setup exist
fd -t f "tsconfig" | head -10Repository: knockout/tko
Length of output: 87
🏁 Script executed:
# Search for Mocha type definitions and test context setup
fd -t f "vitest" -o "mocha" | head -20Repository: knockout/tko
Length of output: 185
🏁 Script executed:
# Check builds/knockout/helpers/browser-setup.js to understand the runtime shim
cat -n builds/knockout/helpers/browser-setup.js | sed -n '95,115p'Repository: knockout/tko
Length of output: 948
🏁 Script executed:
# Check tsconfig.json for type settings
cat tsconfig.jsonRepository: knockout/tko
Length of output: 918
🏁 Script executed:
# Check for any .d.ts files in the spec directories
fd -t f "\.d\.ts" packages/bind/spec builds/*/spec 2>/dev/null | head -20Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Search for Vitest or Mocha configuration
fd -t f "vitest.config|mocha.opts|.mocharc" | head -10Repository: knockout/tko
Length of output: 72
🏁 Script executed:
# Look for type definitions related to test context (this.skip)
rg -t ts -t js "this\.skip" packages/bind/spec --max-count 20Repository: knockout/tko
Length of output: 175
🏁 Script executed:
# Check if there's a package.json with test framework info
cat package.json | jq '.devDependencies | keys[] | select(. | contains("vitest") or contains("mocha") or contains("jest"))'Repository: knockout/tko
Length of output: 126
🏁 Script executed:
# Search for test framework imports in the crossWindowBehaviors file
head -30 packages/bind/spec/crossWindowBehaviors.tsRepository: knockout/tko
Length of output: 814
🏁 Script executed:
# Check vitest.config to understand how tests are run
cat vitest.config.tsRepository: knockout/tko
Length of output: 1293
🏁 Script executed:
# Look at the mocha-test-helpers to understand test context
cat packages/bind/spec/../../../utils/helpers/mocha-test-helpers.ts 2>/dev/null || cat packages/utils/helpers/mocha-test-helpers.ts 2>/dev/null || fd -t f "mocha-test-helpers" | head -3 | xargs catRepository: knockout/tko
Length of output: 2834
🏁 Script executed:
# Check if there's a way to run type checking to confirm TS2554
# Look for npm scripts
cat package.json | jq '.scripts' | head -30Repository: knockout/tko
Length of output: 870
🏁 Script executed:
# Search for any existing type overrides or ambient declarations for Mocha
rg -t ts "declare.*skip" packages builds --max-count 10Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Look at what Mocha's actual skip signature is in the types
# Check node_modules for `@types/mocha` definition if available, or search online knowledge
rg -t d.ts "skip.*function" --max-count 5Repository: knockout/tko
Length of output: 84
🏁 Script executed:
# Check if there are other usages of this.skip with a reason in the test files
rg "this\.skip\(" packages builds --type ts --type js -A 1 | head -20Repository: knockout/tko
Length of output: 731
🏁 Script executed:
# Let's try to run tsc to see if TS2554 error occurs at line 53
cd . && npm run tsc 2>&1 || bunx tsc 2>&1 | grep -A 3 -B 3 "crossWindowBehaviors\|TS2554" | head -30Repository: knockout/tko
Length of output: 97
🏁 Script executed:
# Check what version of `@types/mocha` is being used and its skip signature
cat package.json | jq '.devDependencies."@types/mocha"'Repository: knockout/tko
Length of output: 66
🏁 Script executed:
# Look at the actual Mocha types to understand the skip signature
# Search in node_modules or online knowledge about Mocha skip signature
find node_modules -path "*mocha*/index.d.ts" 2>/dev/null | head -1 | xargs head -100 2>/dev/null | grep -A 5 "skip"Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Check if there are any build artifacts or compiled versions
ls -la builds/knockout/spec/crossWindowBehaviors.* 2>/dev/nullRepository: knockout/tko
Length of output: 149
🏁 Script executed:
# Look at the built/compiled version to see if it's the same
head -60 builds/knockout/spec/crossWindowBehaviors.js | tail -20Repository: knockout/tko
Length of output: 916
🏁 Script executed:
# Let's check the actual `@types/mocha` type definitions for Mocha context
# Search for how skip is defined in the node_modules
find . -type f -name "index.d.ts" -path "*/mocha*" 2>/dev/null | head -1 | xargs cat 2>/dev/null | grep -A 10 "skip"Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Let's look at what the actual Mocha types define for Runnable.skip
python3 << 'EOF'
import json
import subprocess
# Try to find the mocha types in node_modules
result = subprocess.run(['find', '.', '-type', 'f', '-path', '*/node_modules/@types/mocha/index.d.ts'],
capture_output=True, text=True)
if result.stdout:
with open(result.stdout.strip().split('\n')[0], 'r') as f:
content = f.read()
# Find the skip method definition
if 'skip' in content:
lines = content.split('\n')
for i, line in enumerate(lines):
if 'skip' in line.lower() and ('function' in line or '(' in line):
print(f"Line {i}: {line}")
# Print context
for j in range(max(0, i-2), min(len(lines), i+5)):
print(f" {j}: {lines[j]}")
print()
EOFRepository: knockout/tko
Length of output: 38
🏁 Script executed:
# Check if there's a git history showing whether this was recently changed
git log --oneline -10 packages/bind/spec/crossWindowBehaviors.ts 2>/dev/null || echo "Git history not available"Repository: knockout/tko
Length of output: 126
🏁 Script executed:
# Let's check the actual type of 'this' in the test context by looking for type assertions
grep -n "as any\|as unknown\|@ts-" packages/bind/spec/crossWindowBehaviors.ts | head -10Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Check if there's already a type override or if this file has any type annotations
head -40 packages/bind/spec/crossWindowBehaviors.tsRepository: knockout/tko
Length of output: 1132
🏁 Script executed:
# Let's check what Mocha's actual skip signature is documented as
# Search for recent Mocha type information or check npm registry knowledge
python3 << 'EOF'
# Check if we can find any type stubs or see what the actual method signature looks like
import subprocess
# Look for how skip is actually being used elsewhere in the codebase
result = subprocess.run(['rg', 'skip\\(', 'packages/', 'builds/', '--type', 'ts', '--type', 'js', '-B', '1', '-A', '1'],
capture_output=True, text=True)
lines = result.stdout.split('\n')[:40]
for line in lines:
print(line)
EOFRepository: knockout/tko
Length of output: 3240
🏁 Script executed:
# Let's check if the type definitions for Mocha actually allow a reason parameter
# by looking at the current Mocha documentation or `@types/mocha` source
python3 << 'EOF'
# Modern Mocha versions (5.0+) support skip(reason) but older versions don't
# Let's check what version constraint is in package.json
import subprocess
import json
result = subprocess.run(['cat', 'package.json'], capture_output=True, text=True)
if result.returncode == 0:
data = json.loads(result.stdout)
mocha_version = data.get('devDependencies', {}).get('@types/mocha', 'unknown')
print(f"@types/mocha version: {mocha_version}")
# In Mocha 5.0+, the skip function signature is:
# skip(): never (no arguments in older signatures)
# skip(message?: string): never (in newer versions)
# `@types/mocha` 10.0.10 should support skip(message?: string)
print("\nExpected behavior with `@types/mocha` 10.0.10:")
print(" this.skip() should accept optional message parameter")
print(" Type should be: (message?: string) => never")
EOFRepository: knockout/tko
Length of output: 233
🏁 Script executed:
# Let's directly check what the actual type definition says by examining the source if available
cat package.json | grep -A 50 "devDependencies" | grep -E "@types/mocha|vitest|mocha"Repository: knockout/tko
Length of output: 187
🏁 Script executed:
# Check the actual usage in the browser-setup wrapper to confirm it passes reason
sed -n '100,110p' builds/knockout/helpers/browser-setup.jsRepository: knockout/tko
Length of output: 486
🏁 Script executed:
# Let's check if there's any local type override for the Mocha context that might not include the message parameter
fd -t f "\.ts" packages/utils/helpers | xargs grep -l "skip" 2>/dev/nullRepository: knockout/tko
Length of output: 90
🏁 Script executed:
# Check the test-env.ts more carefully to see the recommended pattern
cat packages/utils/helpers/test-env.tsRepository: knockout/tko
Length of output: 443
🏁 Script executed:
# Let's see the actual function signature of the test in crossWindowBehaviors.ts to verify if it receives ctx parameter
sed -n '49,55p' packages/bind/spec/crossWindowBehaviors.tsRepository: knockout/tko
Length of output: 310
🏁 Script executed:
# Check other test files using skip with message to see the pattern
rg "skip\(" packages/utils/spec --type ts -B 2 | head -30Repository: knockout/tko
Length of output: 752
🏁 Script executed:
# Verify if there's a build output or dist version
ls -la packages/bind/dist 2>/dev/null | head -10Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Try to understand if the `@types/mocha` version actually supports skip with message
python3 << 'EOF'
# `@types/mocha` version history:
# - Versions before 5.0: skip() had no parameters
# - Versions 5.0+: skip(message?: string) is supported
# - Version 10.0.10 is recent and should have skip(message?: string)
# However, Vitest might override these types. Let's check if there's a vitest-setup that could affect types
import subprocess
result = subprocess.run(['cat', 'builds/knockout/helpers/vitest-setup.js'],
capture_output=True, text=True)
print(result.stdout)
EOFRepository: knockout/tko
Length of output: 1189
🏁 Script executed:
# Let's verify if we can find any TypeScript error logs or CI output
find . -name "*.log" -o -name "*.txt" | grep -i test 2>/dev/null | head -5Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Check if there's a GitHub Actions or CI config that might show recent test results
fd -t f ".github/workflows" -o ".gitlab-ci.yml" -o ".circleci" 2>/dev/nullRepository: knockout/tko
Length of output: 38
🏁 Script executed:
# Let's verify the actual test pattern - check if crossWindowBehaviors should have ctx parameter
# Compare with other test files that use skip with message
grep -n "it('.*function.*ctx" packages/*/spec/*.ts builds/*/spec/*.ts 2>/dev/null | head -20Repository: knockout/tko
Length of output: 2716
🏁 Script executed:
# The key question: does the function need a ctx parameter or is this.skip supposed to work?
# Let's check if there are any tests using this.skip with message parameter successfully
grep -n "this\.skip(" packages/*/spec/*.ts builds/*/spec/*.ts 2>/dev/nullRepository: knockout/tko
Length of output: 178
🏁 Script executed:
# Let's check if there's a specific TypeScript or Vitest type override that might be causing the issue
fd -t f "tsconfig" -o "vitest.config" -o ".d.ts" | xargs grep -l "skip\|Mocha" 2>/dev/nullRepository: knockout/tko
Length of output: 203
🏁 Script executed:
# Check if `@types/mocha` type definitions are being overridden anywhere
rg "interface.*Context\|interface.*Test\|declare.*skip" packages builds --type ts --max-count 10Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Let's check what the actual issue is - look at how ctx parameter is supposed to work
# by examining a working example from utilsDomBehaviors
sed -n '173,180p' packages/utils/spec/utilsDomBehaviors.tsRepository: knockout/tko
Length of output: 457
🏁 Script executed:
# Most importantly: verify if the test function SHOULD receive ctx parameter
# The browser-setup wrapper at line 104 shows: fn.call(this, { skip: reason => this.skip(reason) })
# This injects the first parameter. Does crossWindowBehaviors accept it?
sed -n '49,60p' packages/bind/spec/crossWindowBehaviors.tsRepository: knockout/tko
Length of output: 490
🏁 Script executed:
# Check if there are any tests that successfully use this.skip(message) without ctx parameter
rg "this\.skip\(" packages builds --type ts --type js -B 3 -A 1 | head -40Repository: knockout/tko
Length of output: 1733
🏁 Script executed:
# Let me verify the key insight: does the test function receive the injected ctx parameter?
# The browser-setup wrapper only injects ctx if fn.length === 1
# Let's check the function length of crossWindowBehaviors test
python3 << 'EOF'
# The test function signature matters:
# function () { } has length 0 - no parameters injected
# function (ctx) { } has length 1 - ctx parameter injected with { skip: ... }
# function (ctx, done) { } has length 2 - no ctx injection
# crossWindowBehaviors has: it('...', function () { ... })
# This has length 0, so NO ctx is injected
# Therefore this.skip is the raw Mocha skip, not the wrapped one with message support
# The solution depends on:
# 1. Does raw Mocha's this.skip(message) work? Yes, in Mocha 5.0+ with `@types/mocha` 10.0.10
# 2. Does TypeScript think it works? No, apparently there's a type mismatch
# Let's verify the actual recommendation by checking the pattern in the codebase
import subprocess
result = subprocess.run(['grep', '-n', 'function (ctx', 'packages/bind/spec/crossWindowBehaviors.ts'],
capture_output=True, text=True)
print("Tests with ctx parameter in crossWindowBehaviors.ts:")
print(result.stdout if result.stdout else "(none found)")
# Check how many tests in that file have ctx vs don't have ctx
result2 = subprocess.run(['grep', '-c', "it('", 'packages/bind/spec/crossWindowBehaviors.ts'],
capture_output=True, text=True)
print(f"\nTotal it() calls: {result2.stdout.strip()}")
EOFRepository: knockout/tko
Length of output: 142
🏁 Script executed:
# The key question: what's the correct idiomatic fix in this codebase?
# Option 1: Add ctx parameter (recommended pattern per test-env.ts)
# Option 2: Use type cast (proposed in review)
# Let's check if other files had similar issues fixed
git log --all --oneline --grep="skip" | head -20Repository: knockout/tko
Length of output: 1296
🏁 Script executed:
# Let's also verify `@types/mocha` 10.0.10 actually supports skip with message
# by checking if we can find any documentation or look at how test-env.ts documents this
cat packages/utils/helpers/test-env.tsRepository: knockout/tko
Length of output: 443
🏁 Script executed:
# Let's check the exact commit that introduced this line to understand the context
git show 7c29892 --statRepository: knockout/tko
Length of output: 39984
🏁 Script executed:
# Look at the full diff of that commit to understand the intent
git show 7c29892 | head -60Repository: knockout/tko
Length of output: 2267
🏁 Script executed:
# Check if there's more context about why this pattern was chosen
git log -p 7c29892~1..7c29892 -- packages/bind/spec/crossWindowBehaviors.ts | head -80Repository: knockout/tko
Length of output: 38
Add ctx parameter to match the established test pattern in this codebase.
The test function should accept the ctx: any parameter, which receives the wrapped skip function that properly supports the message argument. This follows the documented pattern in packages/utils/helpers/test-env.ts and is consistent with other environment-conditional skips throughout the test suite.
🐛 Recommended fix
- it('Should work in another window', function () {
+ it('Should work in another window', function (ctx: any) {
const win2 = window.open('', '_blank', 'height=150,location=no,menubar=no,toolbar=no,width=250')
if (!win2) {
- return this.skip('popup blocked — cross-window assertions cannot run')
+ return ctx.skip('popup blocked — cross-window assertions cannot run')
}🧰 Tools
🪛 GitHub Actions: Lint & Typecheck
[error] 53-53: TypeScript (tsc) error TS2554: Expected 0 arguments, but got 1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bind/spec/crossWindowBehaviors.ts` at line 53, The test callback
currently calls this.skip('popup blocked — cross-window assertions cannot run')
but must accept the ctx: any parameter used across this codebase; change the
test function signature to include ctx (e.g., (ctx: any) => { ... }) and replace
this.skip(...) with ctx.skip('popup blocked — cross-window assertions cannot
run') so the wrapped skip supporting a message is used (follow the pattern in
packages/utils/helpers/test-env.ts and other environment-conditional skips).
Summary
Both cross-window specs (
builds/knockout/spec/crossWindowBehaviors.js:7,packages/bind/spec/crossWindowBehaviors.ts:52) silentlyreturnwhenwindow.openyields null because of the popup blocker. Mocha counts a sync function that throws nothing as a pass, so under any browser with popups blocked the suite went green while every cross-window assertion was actually skipped — a silent coverage gap noticed while running the new/testsrunner on tko.io.Swap the bare
returnforreturn this.skip('popup blocked — cross-window assertions cannot run')so the suite reports these as pending (○) in the stats chip. Blocked-popup state is now visible.Test plan
/testsstill reaches 2708/0/42 in ~5.5s when popups are allowed (no behavioral change).Summary by CodeRabbit