tests(gather-runner): type check #10215
Conversation
Co-Authored-By: Patrick Hulce <[email protected]>
| @@ -29,8 +53,16 @@ declare global { | |||
| /** Make optional all properties on T and any properties on object properties of T. */ | |||
| type RecursivePartial<T> = { | |||
| [P in keyof T]+?: | |||
| // If type is a union, map each individual component and transform the resultant tuple back into a union. | |||
There was a problem hiding this comment.
This was needed for types such as:
export interface Json {
passes: PassJson[] | null;
}It would otherwise not recuse into RecursivePartial<PassJson>, as PassJson[] | null fails all the conditional checks and just returns itself (T[P]).
Note, if the property is optional (and LH.Config.Json['passes'] actually is), SplitTypes<T> turns it into [undefined, PassJson[], null], so no special handling needed for that.
There was a problem hiding this comment.
Can you add a comment in the code with this concrete example? This helped a lot :)
| @@ -787,20 +858,21 @@ describe('GatherRunner', function() { | |||
|
|
|||
| return GatherRunner.run(config.passes, options) | |||
| .then(artifacts => { | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
at this point this feels like an example of a test that is pointless with type checking :)
we could start testing all kinds of properties to make sure they're not set
assert.equal(artifacts.traceOfTab, undefined)
assert.equal(artifacts.BabyYoda, undefined)
There was a problem hiding this comment.
had the same thought, just wanted someone else to notice too :)
| @@ -29,8 +53,16 @@ declare global { | |||
| /** Make optional all properties on T and any properties on object properties of T. */ | |||
| type RecursivePartial<T> = { | |||
| [P in keyof T]+?: | |||
| // If type is a union, map each individual component and transform the resultant tuple back into a union. | |||
There was a problem hiding this comment.
Can you add a comment in the code with this concrete example? This helped a lot :)
| // Guard against large string unions, which would be unreasonable to support (much more than 3 components is common). | ||
| SplitType<T[P]> extends string[] ? T[P] : | ||
| GetLength<SplitType<T[P]>> extends 2|3|4 ? RecursivePartialUnion<T[P]>[number] : | ||
| // Recurse into arrays. |
There was a problem hiding this comment.
these comments are great 👍
| return /** @type {(...args: RecursivePartial<TParams>) => TReturn} */ (fn); | ||
| } | ||
|
|
||
| const GatherRunner = { |
There was a problem hiding this comment.
meta question: do we still end up catching that many type bugs if we're mutating the type signature of the class under test so much?
should we be creating default configs/passes/etc to modify for each instead to maintain the integrity of the test?
There was a problem hiding this comment.
There were no type bugs that were discovered from this particular file. Perhaps other files will have some (IIRC, driver-test had one).
The larger benefit IMO is in writing new tests, modifying existing ones, or just understanding current ones in the IDE (easier with types).
No description provided.