AI-generated test fixes on GJS#1
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughJasmine's implementation and test suite now gracefully handle restricted and varied runtime environments. Core code adds guards before accessing URL and MessageChannel globals. Test suites conditionally skip when required APIs (performance, structuredClone, DOM helpers, console.error spyability) are unavailable. Integration tests introduce a queueMicrotask fallback and thread it through mocked globals. Stack filtering and namespace tests are adjusted for runtime variations. ChangesRuntime Environment Capability Hardening
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/Clock.js (1)
246-264:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
elsekeyword causes both MessageChannel and setTimeout to run sequentially.The conditional block for
MessageChannel(lines 246-258) lacks anelsebefore thesetTimeoutblock (lines 261-263). As written, whenMessageChannelis available, the function awaits on MessageChannel AND then also awaits on setTimeout. The comment on line 247 and the AI summary both indicate that MessageChannel is meant to be an alternative to prevent setTimeout throttling, not an additional step.🔧 Proposed fix to use MessageChannel OR setTimeout
if (typeof MessageChannel !== 'undefined') { // MessageChannel ensures that setTimeout is not throttled to 4ms. // https://developer.mozilla.org/en-US/docs/Web/API/setTimeout#reasons_for_delays_longer_than_specified // https://stackblitz.com/edit/stackblitz-starters-qtlpcc // Note: This trick does not work in Safari, which will still throttle the setTimeout const channel = new MessageChannel(); await new Promise(resolve => { channel.port1.onmessage = resolve; channel.port2.postMessage(undefined); }); channel.port1.close(); channel.port2.close(); + } else { + // setTimeout ensures that we interleave with other setTimeouts. + await new Promise(resolve => { + realTimingFunctions.setTimeout.call(global, resolve); + }); } - - // setTimeout ensures that we interleave with other setTimeouts. - await new Promise(resolve => { - realTimingFunctions.setTimeout.call(global, resolve); - }); }🤖 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 `@src/core/Clock.js` around lines 246 - 264, The current code awaits both the MessageChannel hack and then unconditionally awaits setTimeout; update the control flow so the setTimeout fallback only runs when MessageChannel is unavailable by adding an else branch for the existing if (typeof MessageChannel !== 'undefined') { ... } block. Concretely, keep the MessageChannel await block as-is and move the await new Promise(resolve => { realTimingFunctions.setTimeout.call(global, resolve); }); into an else clause so only one of the two (MessageChannel OR setTimeout) executes; reference the if (typeof MessageChannel !== 'undefined') check and the setTimeout call using realTimingFunctions.setTimeout.call(global, resolve) to locate the lines to change.
🤖 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.
Inline comments:
In `@spec/core/baseSpec.js`:
- Around line 164-174: The console.error spy setup in the block that defines
consoleErrorDescriptor and calls spyOn(console, 'error') handles non-spyable
descriptors by silently skipping spy setup; make it consistent with
DeprecatorSpec.js by marking the spec as pending and returning early when
consoleErrorDescriptor is missing, writable, or has a setter. Update the code
around the consoleErrorDescriptor check to call pending(...) (or this.pending())
and return instead of continuing, ensuring the same guard condition (checking
consoleErrorDescriptor, .writable, or .set) is used so the behavior matches
DeprecatorSpec.js.
In `@spec/core/integration/GlobalErrorHandlingSpec.js`:
- Around line 11-16: In the beforeEach block where you call pending('Global
error handling is not available in this environment'), add an immediate return
after that call to skip the rest of the setup; update the beforeEach that checks
typeof addEventListener/process to call pending(...) and then return so
subsequent setup code (the lines following the pending() call) does not execute
when the spec is pending, matching the pattern used in EnvSpec.js and
DeprecationSpec.js.
---
Outside diff comments:
In `@src/core/Clock.js`:
- Around line 246-264: The current code awaits both the MessageChannel hack and
then unconditionally awaits setTimeout; update the control flow so the
setTimeout fallback only runs when MessageChannel is unavailable by adding an
else branch for the existing if (typeof MessageChannel !== 'undefined') { ... }
block. Concretely, keep the MessageChannel await block as-is and move the await
new Promise(resolve => { realTimingFunctions.setTimeout.call(global, resolve);
}); into an else clause so only one of the two (MessageChannel OR setTimeout)
executes; reference the if (typeof MessageChannel !== 'undefined') check and the
setTimeout call using realTimingFunctions.setTimeout.call(global, resolve) to
locate the lines to change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ba55284-b691-4e7d-9b89-a53d05c15c60
📒 Files selected for processing (19)
lib/jasmine-core/jasmine.jsspec/core/ClockSpec.jsspec/core/DelayedFunctionSchedulerSpec.jsspec/core/DeprecatorSpec.jsspec/core/ExceptionFormatterSpec.jsspec/core/SpecSpec.jsspec/core/SuiteSpec.jsspec/core/baseSpec.jsspec/core/integration/DeprecationSpec.jsspec/core/integration/EnvSpec.jsspec/core/integration/GlobalErrorHandlingSpec.jsspec/core/integration/MatchersSpec.jsspec/core/jasmineNamespaceSpec.jsspec/core/matchers/matchersUtilSpec.jsspec/core/matchers/toEqualSpec.jsspec/core/matchers/toHaveClassSpec.jsspec/core/matchers/toHaveClassesSpec.jssrc/core/Clock.jssrc/core/base.js
📜 Review details
🔇 Additional comments (26)
spec/core/matchers/toHaveClassSpec.js (1)
2-7: LGTM!spec/core/matchers/toHaveClassesSpec.js (1)
2-7: LGTM!spec/core/matchers/toEqualSpec.js (1)
707-710: LGTM!spec/core/integration/MatchersSpec.js (1)
639-644: LGTM!Also applies to: 658-664
spec/core/jasmineNamespaceSpec.js (1)
22-25: LGTM!Also applies to: 30-34, 38-40, 45-48, 53-57, 63-65, 125-125
spec/core/ExceptionFormatterSpec.js (1)
383-385: LGTM!spec/core/integration/EnvSpec.js (4)
4-8: LGTM!
509-515: LGTM!
124-124: LGTM!Also applies to: 205-205, 836-836
2948-2952: LGTM!spec/core/integration/GlobalErrorHandlingSpec.js (3)
3-7: LGTM!
35-35: LGTM!
893-900: LGTM!spec/core/integration/DeprecationSpec.js (2)
5-8: LGTM!
11-14: LGTM!src/core/base.js (1)
180-186: LGTM!lib/jasmine-core/jasmine.js (2)
409-409: LGTM!
3102-3114: LGTM!spec/core/baseSpec.js (2)
14-17: LGTM!
72-77: LGTM!spec/core/ClockSpec.js (1)
860-863: LGTM!spec/core/DelayedFunctionSchedulerSpec.js (1)
328-332: LGTM!spec/core/DeprecatorSpec.js (1)
5-9: LGTM!spec/core/SpecSpec.js (1)
117-120: LGTM!spec/core/SuiteSpec.js (1)
419-422: LGTM!spec/core/matchers/matchersUtilSpec.js (1)
664-706: LGTM!
| const consoleErrorDescriptor = Object.getOwnPropertyDescriptor( | ||
| console, | ||
| 'error' | ||
| ); | ||
| if ( | ||
| !consoleErrorDescriptor || | ||
| consoleErrorDescriptor.writable || | ||
| consoleErrorDescriptor.set | ||
| ) { | ||
| spyOn(console, 'error'); // Node <22 | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Inconsistent handling of non-spyable console.error compared to DeprecatorSpec.js.
This code silently skips spying on console.error when it's not spyable, whereas DeprecatorSpec.js (lines 5-9) marks the test as pending and returns early for the same condition. Both approaches work, but the inconsistency may cause confusion.
Consider aligning the approach: either both files should mark tests as pending, or both should silently skip the spy setup.
🤖 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 `@spec/core/baseSpec.js` around lines 164 - 174, The console.error spy setup in
the block that defines consoleErrorDescriptor and calls spyOn(console, 'error')
handles non-spyable descriptors by silently skipping spy setup; make it
consistent with DeprecatorSpec.js by marking the spec as pending and returning
early when consoleErrorDescriptor is missing, writable, or has a setter. Update
the code around the consoleErrorDescriptor check to call pending(...) (or
this.pending()) and return instead of continuing, ensuring the same guard
condition (checking consoleErrorDescriptor, .writable, or .set) is used so the
behavior matches DeprecatorSpec.js.
| if ( | ||
| typeof addEventListener === 'undefined' && | ||
| typeof process === 'undefined' | ||
| ) { | ||
| pending('Global error handling is not available in this environment'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add early return after pending() for consistency.
The top-level beforeEach lacks a return statement after pending(). Without it, lines 18-19 still execute even when the test is marked as pending, creating unnecessary setup. EnvSpec.js (line 515) and DeprecationSpec.js (line 13) both use early return after pending() to skip remaining setup.
🔄 Proposed fix to add early return
if (
typeof addEventListener === 'undefined' &&
typeof process === 'undefined'
) {
pending('Global error handling is not available in this environment');
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| typeof addEventListener === 'undefined' && | |
| typeof process === 'undefined' | |
| ) { | |
| pending('Global error handling is not available in this environment'); | |
| } | |
| if ( | |
| typeof addEventListener === 'undefined' && | |
| typeof process === 'undefined' | |
| ) { | |
| pending('Global error handling is not available in this environment'); | |
| return; | |
| } |
🤖 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 `@spec/core/integration/GlobalErrorHandlingSpec.js` around lines 11 - 16, In
the beforeEach block where you call pending('Global error handling is not
available in this environment'), add an immediate return after that call to skip
the rest of the setup; update the beforeEach that checks typeof
addEventListener/process to call pending(...) and then return so subsequent
setup code (the lines following the pending() call) does not execute when the
spec is pending, matching the pattern used in EnvSpec.js and DeprecationSpec.js.
amezin
force-pushed
the
7.0-clanker
branch
2 times, most recently
from
7815cdb to
8b35701
Compare
amezin
force-pushed
the
7.0-clanker
branch
2 times, most recently
from
0731214 to
42c2ddd
Compare
amezin
force-pushed
the
7.0
branch
2 times, most recently
from
3f332b7 to
5a21da7
Compare
`setTimeout()` on GJS wraps the callback in another JS function.
…ric IDs GJS's setTimeout returns a GLib timeout source object, not a number. The test 'does not conflict with native timer IDs' assumes numeric IDs and uses the ID as both a loop bound and a toHaveBeenCalledTimes argument, which crashes when the ID is an object. Add a typeof check and skip the test when timer IDs are not numeric.
…dClone is unavailable GJS does not implement structuredClone. Jasmine's fallback structuredClone implementation (from @ungap/structured-clone) does not reject Promise objects the way native structuredClone does. This caused the 'throws if the value is not structured-cloneable' tests to fail with: Expected function to throw an Error. Add a typeof structuredClone guard to skip these tests in environments without native structuredClone support.
GJS adds internal frames like 'setTimeout/source' and 'GLib.MainLoop.prototype.runAsync' to stack traces. The test 'includes all aggregated errors in the stack trace' expects exactly 10 lines after filtering, but in GJS there are extra frames that the existing filters don't catch. Add a filter for 'resource:///org/gnome/gjs/' to exclude GJS internal frames from the line count check.
1 participant
No description provided.