Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates several browser test files from JavaScript to TypeScript by removing @ts-nocheck directives and adding proper type definitions. The migration ensures type safety across browser tests while maintaining existing test functionality.
Key changes:
- Consolidated global Window interface type definitions into a shared test utilities file
- Added proper TypeScript type annotations to test helper functions
- Fixed
setTimeoutcalls to match TypeScript's expected signature
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/browser.rollbar.test-utils.ts | Added global Window interface extensions including rollbar, server, chrome, and _rollbarURH properties |
| test/browser.rollbar.autoInstrument.test.ts | Updated setTimeout calls to include the required second parameter for TypeScript compatibility |
| test/browser.rollbar.autoInstrument.fetch.test.ts | Updated setTimeout calls to include the required second parameter for TypeScript compatibility |
| test/browser.rollbar.autoInstrument.csp.test.ts | Removed @ts-nocheck directive and updated setTimeout call signature |
| test/browser.replay.recorder.test.ts | Removed @ts-nocheck directive and refactored Recorder instantiation to use options object pattern |
| test/browser.predicates.test.ts | Removed @ts-nocheck directive to enable TypeScript checking |
| test/browser.domUtility.test.ts | Removed @ts-nocheck, added local Node interface, and refactored genElement function with proper TypeScript typing |
| test/browser.core.test.ts | Removed duplicate Window interface declarations now centralized in test-utils |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
matux
force-pushed
the
matux/migrate-test-js-to-ts-3
branch
from
07c29b4 to
46344ae
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const causeErr = new Error('cause error'); | ||
| const err = new Error('test error', { cause: causeErr }); | ||
| const err = new Error('test error') as Error & { cause?: Error }; | ||
| err.cause = causeErr; |
There was a problem hiding this comment.
The options argument with cause isn't working?
There was a problem hiding this comment.
Fixed: 7b3ef9b
Tell me what do you think of this.
test/browser.telemetry.test.ts
Outdated
|
|
||
| it('should handle select type input events', async function () { | ||
| const elem = document.getElementById('fruit-select'); | ||
| const elem = getElementById('fruit-select'); |
There was a problem hiding this comment.
Did these not work with document.?
| rollbar.log('test'); // generate a payload to inspect | ||
|
|
||
| await setTimeout(1); | ||
| await setTimeout(1, null); |
There was a problem hiding this comment.
It should be valid to call setTimeout with one argument. Was this now working?
matux
force-pushed
the
matux/migrate-test-js-to-ts-3
branch
from
e080121 to
7b3ef9b
Compare
matux
force-pushed
the
matux/migrate-test-js-to-ts-3
branch
from
ed506bc to
1f5d190
Compare
2 participants
Description of the change
This PR migrates all the browser tests to TS.