Feature request: Support snapshot testing #14936
Comments
fisker
added
core
|
Can you provide an example of the type of test that you would like to write? |
|
Sorry, what I meant was, can you show how you are currently using SourceCodeFixer? My preference is not to expose that as an API, but I’m curious if the pattern you were using is something we should just build into RuleTester. Please provide as much detail as possible to help us evaluate this. |
|
Here is the line I use it, I've linked in the description. Update: I'm sorry that I forgot to mention this is request to assert output for suggestions. |
|
Ah! I understand now, thanks for explaining. So you are wanting to make sure that each suggestion, when applied, is outputting the correct result? If so, that seems like something we should build into |
That's right.
I'm afraid it won't. eslint/lib/rule-tester/rule-tester.js Lines 928 to 944 in 81c60f4 As mentioned in the issue description, unless we can customize the assertion step
|
|
Hmm, okay. Well, I still think the correct solution here is to figure out how to modify And please be as detailed as you can. |
|
Note to self (mostly): You can currently test suggestions in |
|
I use a hack to be able to use snapshot testing in And the hack looks like so: There are some things very specific to that plugin, but the gist is that it replace |
|
@lydell thanks, that’s helpful. I think it’s clear people want snapshot testing, so I’ll ask you the same question: what would we need to add to RuleTester that would eliminate your hacks? |
|
This change would do it for me: -output: string,
+output: string | ((actual: string) => void),Currently, you can set My proposal would be that you could optionally set With that, I could remove my hack and all I would have to do to my tests would be this (which would be totally fine): {
code: input`
|import x2 from "b"
|import x1 from "a";
`,
output: (actual) => {
- expect(actual).toMatchInlineSnapshot(`
+ expect(format(actual)).toMatchInlineSnapshot(`
|import x1 from "a";
|import x2 from "b"
`);
},
errors: 1,
},(The |
|
Okay, so it sounds like what is needed is a way to use a custom assert to compare the actual output to the expected output. That’s seems like a reasonable addition. @eslint/eslint-tsc this seems like something we should address before the final v8 is released. |
It’s a bit difficult for me to understand what this means. To me it sounds a bit like |
|
So here's what I'm thinking: {
code: "foo;",
output: "bar;",
outputAssert(actual, expected) {
// do what you want here
}
}Notes on this:
So, you could do this: {
code: "foo;",
output: "bar;",
outputAssert: assert.strictEqual
}Or you could do this: {
code: "foo;",
outputAssert(actual) {
expect(actual).toMatchInlineSnapshot("bar;");
}
}Or you could do this: {
code: "foo;",
output: "bar;",
outputAssert(actual, expected) {
expect(actual).toMatchInlineSnapshot(expected);
}
} |
|
Sounds good to me! (For the record, nobody would do your last example in practice, but it is possible.) |
@fisker can you apply the fix manually? It should be as simple as: const { fix } = suggestion;
const output = `${code.slice(0, fix.range[0])}${fix.text}${code.slice(fix.range[1])}`;Applying multiple fixes has additional logic due to possible overlapping, so that would be indeed complicated. For a single fix, it's just replacing the range with the given text, as described in EditInfo type, e.g., integrations are expected to manually apply suggestions. |
|
@nzakas Awesome! Do you have any tips on how to try your code? I attempted this:
I get this error: The I tried upgrading Jest from v26 to v27, and got this error instead: I’m a bit unsure if:
|
|
Try removing the eslint package first and then reinstalling from the PR: npm remove eslint —save-dev |
|
Thanks for working on this. I'm afraid it won't work for my case, as I understand, the As you can see in this snapshot, there is a code frame pointing the report range, this is the inital propose adding this custom tester, because it's hard to test the report range using the How about pass all the ruleTester.run("foo", replaceProgramWith5Rule, {
valid: [],
invalid: [
{
code: "var foo = bar;",
output: "5",
errors: 3,
assert(actual, scenario) {
assert.strictEqual(actual.output, scenario.output, 'Message');
// Or maybe we can use `this`, but not important
// assert.strictEqual(actual.output, this.output, 'Message');
assert.strictEqual(actual.messages.length, scenario.errors, 'Message');
},
}
]
});Also, @lydell need ruleTester.run("foo", replaceProgramWith5Rule, {
assertOutput() {
// ...
},
valid: [],
invalid: [
{
code: "var foo = bar;",
output: "5",
errors: 3,
}
]
}); |
nzakas
added
the
needs design
|
@fisker we're trying to figure out if this is a blocker for v8.0.0. Could you please try this solution: #14936 (comment) and tell us whether or not it works for your use case? |
- const {output} = SourceCodeFixer.applyFixes(code, [suggestion]);
+ const { fix } = suggestion;
+ const output = `${code.slice(0, fix.range[0])}${fix.text}${code.slice(fix.range[1])}`; |
@lydell can you try this: // ------- jest.config.js -------------------
module.exports = {
"moduleNameMapper": {
"@eslint/eslintrc/universal": "@eslint/eslintrc/dist/eslintrc-universal.cjs"
}
}I found this suggestion in jestjs/jest#11100 |
|
I've copied the whole |
|
@mdjermanovic Thank you very much! Adding that to Now that I can run my tests, I can report that … |
|
@lydell great! We are going to hold off until after v8.0.0 to look at this further. @fisker I don’t think a single assert method is a good idea. Anyone using that would need to know all the different types of asserts RuleTester already does and remember to check them all. Another option might be to add separate methods, so outputAssert, errorCountAssert, errorAssert, etc. In the near term I’m shifting focus back to v8.0.0, but if anyone else wants to put together an RFC for a change here, please feel free. |
|
@mdjermanovic Your suggestion works as expected. fisker/eslint-plugin-unicorn@5e2c671 |
Great, thanks for the info! Since that works, it seems that there are no blocking issues for v8.0.0 here. |
mdjermanovic
changed the title
SourceCodeFixer
|
Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update. |
|
We still want to take a look at this and would be grateful if someone wants to put together an RFC. |
|
FYI: I am hoping to work on this / write an RFC in the near future. We have snapshot testing in a related linter I work on, ember-template-lint, which could be useful as prior art: |
|
Looks like this isn't planned yet. However, node:test already supports snapshot testing experimentally. I have implemented a const assert = require('node:assert')
const { ESLint } = require('eslint')
const { codeFrameColumns } = require('@babel/code-frame')
const outdent = require('outdent')
const DEFAULT_SNAPSHOT = {
assert: {
snapshot: message => {
assert.fail(message)
}
}
}
const DEFAULT_CONFIG = {
languageOptions: {
ecmaVersion: 'latest',
parserOptions: {
sourceType: 'module'
}
}
}
class TestRunner {
/**
*
* @param {*} baseConfig
* @param {import('node:test').TestContext} t
*/
constructor(baseConfig = DEFAULT_CONFIG, t = DEFAULT_SNAPSHOT) {
this.t = t
this.baseConfig = baseConfig
}
/**
*
* @param {string} ruleName
* @param {import('eslint').Rule.RuleModule} rule
* @param {{
* valid: Array<string | import('eslint').RuleTester.ValidTestCase> | undefined,
* invalid: Array<string | import('eslint').RuleTester.InvalidTestCase> | undefined
* }} tests
* @returns {Promise<void>}
*/
async run(ruleName, rule, tests) {
const { valid = [], invalid = [] } = tests
const failMessages = []
const snapshot = []
// Run valid test cases
for (const [index, validCase] of valid.entries()) {
const code = typeof validCase === 'string' ? validCase : validCase.code
const options = typeof validCase === 'string' ? undefined : validCase.options
const message = await this.runSingleTest(index, ruleName, rule, code, options)
if (message) {
failMessages.push(
outdent`
The following test case should have passed but failed.
${outdent`${message}`}
`
)
}
}
// Run invalid test cases
for (const [index, invalidCase] of invalid.entries()) {
const code = typeof invalidCase === 'string' ? invalidCase : invalidCase.code
const options = typeof invalidCase === 'string' ? undefined : invalidCase.options
const message = await this.runSingleTest(index, ruleName, rule, code, options)
if (message.length > 0) {
snapshot.push(message)
} else {
failMessages.push(
outdent`
The following test case should have failed but passed.
${wrapCodeFrame(code)}
`
)
}
}
if (failMessages.length > 0) {
assert.fail(failMessages.join('\n\n'))
} else {
this.generateSnapshot(snapshot.join('\n\n'))
}
}
/**
*
* @param {number} index
* @param {string} ruleName
* @param {import('eslint').Rule.RuleModule} rule
* @param {string} code
* @param {Array<Record<string, any>>} [options]
* @returns {Promise<string>}
*/
async runSingleTest(index, ruleName, rule, code, options = []) {
const eslint = new ESLint({
cache: false,
overrideConfigFile: true,
baseConfig: {
...this.baseConfig,
rules: {
[`snapshot-tester/${ruleName}`]: ['error', ...options].filter(Boolean) // 动态注入规则
}
},
plugins: {
'snapshot-tester': { rules: { [ruleName]: rule } } // 自定义规则注入
}
})
const results = await eslint.lintText(code)
return results
.map(result => this.generateOutput(index, code, result.messages))
.filter(Boolean)
.join('\n\n')
}
/**
*
* @param {number} index
* @param {string} code
* @param {Array<import('eslint').Linter.LintMessage>} messages
* @returns {string | void}
*/
generateOutput(index, code, messages) {
if (messages.length === 0) return
const title = removeNewlines(code)
/** @type {Array<string>} */
const output = [
`## invalid(${index + 1}): ${title}`,
indent(
outdent`
> Input
${indent(wrapCodeFrame(code), 4)}
`,
2
)
]
for (const [i, message] of messages.entries()) {
output.push(...this.generateErrorMessage(i, messages.length, code, message))
}
return output.join('\n\n')
}
/**
* Generate error message
* @param {number} index
* @param {number} totalLength
* @param {string} code
* @param {import('eslint').Linter.LintMessage} message
* @returns {string[]}
*/
generateErrorMessage(index, totalLength, code, message) {
const { line, column, endLine, endColumn } = message
const frame = codeFrameColumns(code, { start: { line, column }, end: { line: endLine, column: endColumn } })
const output = [
message.fix
? indent(
outdent`
> Output
${indent(wrapCodeFrame(applySingleFix(code, message.fix)), 4)}
`,
2
)
: null,
indent(
outdent`
> Error ${index + 1}/${totalLength}: ${message.message}
${indent(frame, 4)}
`,
2
)
].filter(Boolean)
if (message.suggestions?.length) {
output.push(
indent(
outdent`
${message.suggestions
.map((suggestion, i) => {
const fix = suggestion.fix
// Apply fix
const fixedCode = applySingleFix(code, fix)
return outdent`
> Suggestion ${i + 1}/${message.suggestions.length}: ${suggestion.desc}
${indent(wrapCodeFrame(fixedCode), 4)}
`
})
.join('\n\n')}
`,
2
)
)
}
return output
}
/**
* Generate snapshot file
* @param {string} message
*/
generateSnapshot(message) {
this.t?.assert.snapshot(message, {
serializers: [value => value]
})
}
}
/**
*
* @param {string} str
* @returns
*/
function removeNewlines(str) {
return str.replace(/[\n\r]+/g, String.raw`\n`) // 将所有换行符删除
}
/**
*
* @param {string} code
* @returns
*/
function wrapCodeFrame(code) {
return codeFrameColumns(code, { start: { line: 0, column: 0 } }, { linesAbove: Number.MAX_VALUE, linesBelow: Number.MAX_VALUE })
}
/**
*
* @param {string} code
* @param {import('eslint').Rule.Fix} fix
*/
function applySingleFix(code, fix) {
return replaceStringWithSlice(code, fix.range[0], fix.range[1], fix.text)
}
/**
*
* @param {string} str
* @param {number} start
* @param {number} end
* @param {string} replacement
* @returns {string}
*/
function replaceStringWithSlice(str, start, end, replacement) {
return str.slice(0, start) + replacement + str.slice(end)
}
/**
*
* @param {string} input
* @param {number} count
* @returns
*/
function indent(input, count) {
const indentation = ' '.repeat(count)
return input
.split('\n')
.map(line => `${indentation}${line}`)
.join('\n')
}
module.exports = TestRunnerThen run the test case with the following command node --test --experimental-test-snapshots --test-update-snapshots ./react-prefer-props.test.jsconst test = require('node:test')
const outdent = require('outdent')
const SnapshotTester = require('../../test/SnapshotTester')
const rule = require('./react-prefer-props')
test('react-prefer-props', async t => {
const ruleTester = new SnapshotTester(
{
languageOptions: {
ecmaVersion: 'latest',
parserOptions: {
sourceType: 'module'
}
}
},
t
)
await ruleTester.run('react-prefer-props', rule, {
valid: [
// Valid cases
'function App(props) {}',
'function App({}) {}',
'function App(name) {}',
'var App = () => {}',
// not for async function
'async function App(prop) {}',
// not for generator function
'function* App(prop) {}',
// not for multiple parameters
'function App(prop, other) {}',
// not for no parameters
'function App() {}'
],
invalid: [
// Invalid cases
'function App(prop) {}',
'const App = function (prop) {}',
'const App = (prop) => {}',
outdent`
const App = (prop) => {
console.log(prop)
}
console.log(prop)
`
]
})
})And then it will generate snapshot file |
We test rules with a custom rule tester doing a snapshot test.
The built-in RuleTester can't customize the output assertion, we don't have the output to send to
RuleTester, so we have to useSourceCodeFixerto apply fix frommessages. Full implementationWe definitely want a better way to do snapshot test, maybe pass an
assertoption in future, but before that, can we exposeSourceCodeFixer, so I can apply fix from the message?The version of ESLint you are using.
8.0.0-beta.0
The problem you want to solve.
Fix our rule tester.
Are you willing to submit a pull request to implement this change?
Yes.
The text was updated successfully, but these errors were encountered: