Improve verify.codeFixAvailable #24325
Conversation
ghost
requested review from
src/harness/fourslash.ts
Outdated
| if (negative) { | ||
| if (codeFixes.length) { | ||
| this.raiseError(`verifyCodeFixAvailable failed - expected no fixes but found ${codeFixes.map(c => c.description)}.`); | ||
| assert.equal(info.length, codeFixes.length); |
There was a problem hiding this comment.
My experience has been that asserts like this tend to produce unhelpful output when they fail. I haven't found a silver bullet but sometimes I break verification into two steps - the first determines whether or not verification succeeded and the second produces a human-readable representation of the difference between the expected and actual values.
To be more concrete, if you get fewer fixes than expected, I think this will produce a message like "expected 4 but got 3". It would be much more helpful to know which fixes were actually found or, better still, which were missing.
| @@ -3,4 +3,4 @@ | |||
| // @noImplicitAny: true | |||
| //// function f(y, z = { p: y[ | |||
|
|
|||
| verify.not.codeFixAvailable(); | |||
| verify.codeFixAvailable([]); | |||
There was a problem hiding this comment.
verify.not appears to be idiomatic in this test framework, so I have no problem leaving it even if it doesn't add expressiveness.
src/harness/fourslash.ts
Outdated
| @@ -2997,30 +2997,18 @@ Actual: ${stringify(fullActual)}`); | |||
| } | |||
| } | |||
|
|
|||
| public verifyCodeFixAvailable(negative: boolean, info: FourSlashInterface.VerifyCodeFixAvailableOptions[] | undefined) { | |||
| public verifyCodeFixAvailable(info: FourSlashInterface.VerifyCodeFixAvailableOptions[]) { | |||
There was a problem hiding this comment.
I think you also want to move this to explicit Verify implementation to avoid having it on verify.not
(Move to https://github.com/Microsoft/TypeScript/pull/24325/files#diff-cd92e8371fce86965331cb68f53306f1L4134)
There was a problem hiding this comment.
Since @amcasey requested it, I added back verify.not.
…ctsEqual` for better errors
| this.assertObjectsEqual(fix.commands, info.commands); | ||
| }); | ||
| } | ||
| const actuals = codeFixes.map((fix): FourSlashInterface.VerifyCodeFixAvailableOptions => ({ description: fix.description, commands: fix.commands })); |
There was a problem hiding this comment.
What about change locations?
There was a problem hiding this comment.
This method verify.codeFixAvailable just tests for availability -- verify.codeFix will test the actual content of the fix.
verify.not.codeFixAvailable()as it is equivalent toverify.codeFixAvailable([]);.