chore: migrate some tests to TS #10073
Conversation
ghostd
force-pushed
the
migrate-some-tests
branch
from
b7004df
to
f297c83
Compare
ghostd
force-pushed
the
migrate-some-tests
branch
from
f297c83
to
fa520b3
Compare
| @@ -15,21 +15,22 @@ const jestExpect = require('../'); | |||
| expect.addSnapshotSerializer(alignedAnsiStyleSerializer); | |||
|
|
|||
| jestExpect.extend({ | |||
| toBeDivisibleBy(actual, expected) { | |||
| toBeDivisibleBy(actual: number, expected: number) { | |||
There was a problem hiding this comment.
I suppose in a real matcher actual would always be unknown, but here for simplicity we can ignore that
There was a problem hiding this comment.
If i use unknown, TS does not allow the %operation on the next line (i guess i should add a if typeof before)
There was a problem hiding this comment.
Yes I understand, it was more of a loud thought. Could introduce a type check so that this would be more of a 'proper' matcher implementation. In a test where it's just supposed to be some example matcher, I'd be fine with keeping it as is. Your call 🙃
There was a problem hiding this comment.
Ok, i didn't understand your comment. You mean something like that:
toBeDivisibleBy(actual: unknown, expected: number) {
let pass = false;
if (typeof actual === 'number') {
pass = actual % expected === 0;
}
const message = pass
? () => `expected ${actual} not to be divisible by ${expected}`
: () => `expected ${actual} to be divisible by ${expected}`;
return {message, pass};
},| @@ -34,11 +36,11 @@ describe('isError', () => { | |||
| }); | |||
|
|
|||
| it('should detect errors from another context', () => { | |||
| testErrorFromDifferentContext(win => new win.Error()); | |||
| testErrorFromDifferentContext((win: any) => new win.Error()); | |||
There was a problem hiding this comment.
Window or typeof window or something could be a good type here?
There was a problem hiding this comment.
Window (as defined in lib.dom.ts) does not have Error property:
TS2339: Property 'Error' does not exist on type 'Window'.
We could extend the Window declaration, or use the NodeJS.Global type. Any preferences?
There was a problem hiding this comment.
That is weird. It works on the playground Playground Link
There was a problem hiding this comment.
You are right. The TS error only shows up in my IDE... so i guess my IDE loads the TS settings badly :-/
| @@ -6,8 +6,6 @@ | |||
| * | |||
| */ | |||
|
|
|||
| 'use strict'; | |||
|
|
|||
| const {alignedAnsiStyleSerializer} = require('@jest/test-utils'); | |||
There was a problem hiding this comment.
can we use import in these tests instead of require?
|
@jeysal merge if good to go? |
| @@ -13,14 +13,14 @@ import {isError} from '../utils'; | |||
|
|
|||
| // Copied from https://github.com/graingert/angular.js/blob/a43574052e9775cbc1d7dd8a086752c979b0f020/test/AngularSpec.js#L1883 | |||
| describe('isError', () => { | |||
| function testErrorFromDifferentContext(createError) { | |||
| function testErrorFromDifferentContext(createError: Function) { | |||
There was a problem hiding this comment.
The Function type should be avoided, because it doesn't do any checking at call time, and accepts "functions" like constructors (which will error if not called with new).
It's not blocking yet, but @typescript-eslint added Function to the recommended types for ban-types in v3, so the less new usages of the Function type are added the codebase, the less work I have to do for #10177 :)
You should at worse be able to use (...unknown[]) => any as a quick way to make linting happy.
There was a problem hiding this comment.
Fixed.
Let me know if you have other blockers for the eslint migration.
ghostd
force-pushed
the
migrate-some-tests
branch
from
fea6ed8
to
b84ce97
Compare
|
@jeysal ping |
|
@jeysal hiya 😀 |
|
Thanks @ghostd! |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Migrate some tests