[rule-tester.js] checking error mesasges

[aaronabramov]

right now, if i want to test error messages returned by the linter i do this:

    {code: 'class A {}', errors: [{message: '111'}]}

If the test fails, it'll result into the following error

AssertionError: Error message should be 111

that has no information about what message actually was there.

I assume this is the line that makes the assertion https://github.com/eslint/eslint/blob/master/lib/testers/rule-tester.js#L407

would it be reasonable to add a better description and possibly a regex matching?

I made a rule that checks for existence of test files corresponding to the file that's being linted, what i want to do ideally is:

{
  code: 'class VeryImportantClass {}', 
  errors: [
    {
      message: /^because this file is important it should have a test file at .*\-test\.js$/
    }
  ]
}

[nzakas]

It sounds like you're asking for two things. Can you please open a separate issue for regex matching?

I think what you're asking for otherwise is to output actual and expected values? I could have sworn it did this by default already.

[nzakas]

Related: #6106

[aaronabramov]

@nzakas i think it's mocha integration that provides a nice expected/actual reporting thing. If used outside mocha it just prints expected X to be Y with no actual value.

[aaronabramov]

@nzakas would that work #6533 ?

[nzakas]

Ah, I see. How are you using RuleTester?

[aaronabramov]

i run it with jest and it seems like it automatically hooks it up to jest's global it

[nzakas]

Yeah, it uses whatever global it is available. I'm just wondering if it would be better to follow-up with them so that equality assertions output actual and expected rather than cramming all that info into the message (thus duplicating it for Mocha users).

[platinumazure]

@nzakas Could we support a RuleTester property (e.g., RuleTester.useVerboseErrors) to include/exclude in the error message, similar to how we support defining RuleTester.describe/RuleTester.it? Then jest users could just set that up in a before-all hook somewhere.

[aaronabramov]

I think this way RuleTester would be sort of coupled to a specific testing framework implementation.
It's rather a nice mocha feature to extract and pretty print the expected/actual information from AssertionError, but jest/ava/jasmine don't have this feature, and neither does RuleTester if used outside of any testing environment.

[nzakas]

@DmitriiAbramov I'm not sure if you're replying to me or @platinumazure, so hard to respond.

@platinumazure we could, I'm just not sure it's worth it.

My overall thought here is that it's the testing frameworks who should be responsible for pretty-displaying actual vs. expected values rather than us trying to cram all that information into the message. RuleTester is really just an assertion engine for ESLint that uses the built-in assert module.

Here's an example of what I see in Mocha:

1) max-len invalid function foo() {
    var a; /*this line has 40 characters
    and this one has 36 characters*/
}:

      AssertionError: Error message should be Line x exceeds the maximum line length of 39.
      + expected - actual

      -Line 2 exceeds the maximum line length of 39.
      +Line x exceeds the maximum line length of 39.

To me, that error message is already too long but the showing of the actual vs. expected is the useful part (provided by Mocha).

This still seems to me like an issue with testing frameworks rather than RuleTester.

@eslint/eslint-team other thoughts?

[btmills]

This still seems to me like an issue with testing frameworks rather than RuleTester.

I agree with @nzakas, any changes to this would be extra complexity to work around something that should really be in the testing framework.

[aaronabramov]

as an alternative i can propose to remove RuleTester custom messages at all. When using mocha, failing tests will get an output similar to what @nzakas posted and when using as a standalone tester it'll use asserts default messages, which have both actual and expected values in its output

> const expected = 'EXPECTED', actual = 'ACTUAL'; assert.equal(expected, actual);
AssertionError: 'EXPECTED' == 'ACTUAL'
    at repl:1:56
    at REPLServer.defaultEval (repl.js:269:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:439:12)
    at emitOne (events.js:95:20)
    at REPLServer.emit (events.js:182:7)
    at REPLServer.Interface._onLine (readline.js:211:10)
    at REPLServer.Interface._line (readline.js:550:8)
    at REPLServer.Interface._ttyWrite (readline.js:827:14)
>

So basically. if decided to delegate the formatting to testing frameworks, let's not change the default message given by assert by hiding the actual value. :)

what's going on in rule tester right now is:

> const expected = 'EXPECTED', actual = 'ACTUAL'; assert.equal(expected, actual, `expected should be ${actual}`);
AssertionError: expected should be ACTUAL
    at repl:1:56
    at REPLServer.defaultEval (repl.js:269:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:439:12)
    at emitOne (events.js:95:20)
    at REPLServer.emit (events.js:182:7)
    at REPLServer.Interface._onLine (readline.js:211:10)
    at REPLServer.Interface._line (readline.js:550:8)
    at REPLServer.Interface._ttyWrite (readline.js:827:14)

and it's a pain to work on error message testing without using mocha.

Whatever the decision is, i'm more than happy to submit a PR

[nzakas]

Huh, that's interesting. Nice research! I can get behind that.

Other thoughts?

[aaronabramov]

@nzakas awesome! if you're ok with that, i'll send a PR to use default error messages.

[nzakas]

We need to get TSC approval. @eslint/eslint-tsc does anyone object to removing our custom assert message so the actual and expected values are shown automatically?

[ilyavolodin]

Sounds like a good plan to me. Even using mocha I don't always get expected/actual, sounds like that should fix it.

[nzakas]

Cool, let's do it. @DmitriiAbramov if you want to submit a PR, we're ready for it now.