Generate tests in dtslint with Vanilla#1284
Generate tests in dtslint with Vanilla#1284JanKrivanek wants to merge 1 commit intomicrosoft:mainfrom
Conversation
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
I made some specific comments. Overall the tests that were written were good, but there's a strong preference for testing pure functions, even where the value is low. Nearly all pure functions get tests, a few simple side-effecting functions get tests, and no complex side-effecting functions get tests.
| } | ||
|
|
||
| function normalizePath(file: string) { | ||
| /** @internal */ |
There was a problem hiding this comment.
@internal doesn't actually mean anything in Typescript. It's a tag used by the typescript team to build the typescript compiler but nowhere else. It doesn't mean anything more than "please don't use this" here, which I guess is O.K., but still a little misleading.
| }); | ||
|
|
||
| describe("checkTsconfig - lib", () => { | ||
| it("errors when lib is missing", () => { |
There was a problem hiding this comment.
I think a human might also over-divide the describe sections, but the AI definitely should have put all the checkTsconfig it assertions into one section.
| }); | ||
|
|
||
| describe("checkTsconfig - types with entries", () => { | ||
| it("errors when types array has entries", () => { |
There was a problem hiding this comment.
should also check missing case and empty case (I think they're both success cases, so maybe they're checked elsewhere?)
| expect(checkTsconfig(based({ noUncheckedIndexedAccess: true }))).toEqual([]); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
no tests for arethetypeswrong running or the function that matches @types/X -> X and retrieves X from npm. these are the hard ones since they require mocking a bunch of stuff.
| import path from "path"; | ||
| import os from "os"; | ||
|
|
||
| describe("createProgram", () => { |
| }); | ||
| }); | ||
|
|
||
| describe("combineErrorsAndWarnings", () => { |
There was a problem hiding this comment.
these tests are nice but extremely low-value. If this were part of a human PR adding some feature, I'd tell them not to bother. Since the task for the AI presumably was "fill in missing test coverage", it kind of makes sense here.
| }); | ||
| }); | ||
|
|
||
| describe("checkExpectedFiles", () => { |
There was a problem hiding this comment.
these tests are good; I remember having a couple of bugs in this function that didn't turn up in the initial full run on DT, but were found by humans later.
|
|
||
| const result = checkExpectedFiles(versionDir, true); | ||
| expect(result.errors).toContainEqual(expect.stringContaining("should contain /v16/")); | ||
| }); |
There was a problem hiding this comment.
doesn't check that tslint.json is banned
| expect(result.errors).toContainEqual(expect.stringContaining("should contain /v16/")); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
the difficult functions from index, testTypesVersion and runTests, are not tested. It's not too bad because mostly they call out to other functions, but runTests does have some complex checks that would be good to verify.
| import { testNoLintDisables, normalizePath, isTypesVersionPath, startsWithDirectory, range } from "../src/lint"; | ||
| import { TypeScriptVersion } from "@definitelytyped/typescript-versions"; | ||
|
|
||
| describe("lint", () => { |
There was a problem hiding this comment.
good organisation but no tests for getSourceFiles or anything else that touches eslint. just the pure functions.
2 participants
No description provided.