Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message for differing output in RuleTester #8173

Closed
kentcdodds opened this issue Mar 1, 2017 · 12 comments
Closed

Improve error message for differing output in RuleTester #8173

kentcdodds opened this issue Mar 1, 2017 · 12 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion

Comments

@kentcdodds
Copy link
Contributor

Right now the assertion looks like this

assert.equal(fixResult.output, item.output, "Output is incorrect.");

Which results in not very helpful output in test runs:

 FAIL  templates/01_eslint-simple-demo.test.js
  ● no-blockless-if › invalid › if (foo) foo()

    AssertionError: Output is incorrect.

It would be much more helpful if the output were more like:

 FAIL  templates/01_eslint-simple-demo.test.js
  ● no-blockless-if › invalid › if (foo) foo()

    AssertionError: Output is incorrect.

    Expected value to be:
      "if (foo) {
      foo()
    }"
    Received:
      "if (foo) foo()"

This is similar to what Jest's output is. And perhaps @cpojer can give us a little direction on how to make that kind of output using one of Jest's modules.

I'm happy to contribute this fix. I expect it to be relatively low-risk + high-impact

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Mar 1, 2017
@cpojer
Copy link

cpojer commented Mar 1, 2017

If you are using Jest, you could use expect :)

We don't have the assertion stuff super well abstracted. jest-matcher-utils may work for you, though, it has a bunch of good stuff.

@not-an-aardvark
Copy link
Member

Improving the output here sounds good to me, but I'm confused because I already get output similar to what you're describing:

@not-an-aardvark not-an-aardvark added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Mar 1, 2017
@not-an-aardvark
Copy link
Member

Actually, it looks like that's a Mocha feature (docs):

Mocha supports the err.expected and err.actual properties of any thrown AssertionErrors from an assertion library. Mocha will attempt to display the difference between what was expected, and what the assertion actually saw.

So if you're overriding RuleTester.describe and RuleTester.it, the auto-diff won't happen unless the test runner supports it.

I suppose we could do the diff-creation within eslint, but is there a reason that other test runners don't follow mocha's behavior here?

@ilyavolodin
Copy link
Member

This issue has been raised at least 2-3 times before. Problem is, it seems to work differently for different people. As @not-an-aardvark showed, it works fine for him, but for me, I never get actual/expected.

@kentcdodds
Copy link
Contributor Author

I guess it depends on which testing framework you're using. Perhaps I could change this to investigate supporting Jest?

@kentcdodds kentcdodds reopened this Mar 1, 2017
@kentcdodds
Copy link
Contributor Author

Closed by mistake....

Maybe @cpojer can advise us. Is err.expected and err.actual properties of throw AssertionErrors something that Jest could handle? Or should we make changes to ESLint's code to improve support for Jest?

I guess my question is, where should the changes happen?

I'm more in favor of having Jest support the assert module a little more cleanly by showing (and formatting) the expected and actual properties...

@ilyavolodin
Copy link
Member

@kentcdodds That's the thing, I don't think it's framework related. I never use anything other then mocha, and I don't get actual/expected either... No idea what this might be related to.

@platinumazure
Copy link
Member

@ilyavolodin What happens if you uninstall and reinstall mocha? I'm conjecturing that we haven't bumped our Mocha devDependency in years, and since you're one of the earlier contributors to ESLint, maybe I just happen to have a version of Mocha that actually displays the assert diff...? Obviously I'm reaching here. (I also noticed that our Mocha dependency is ^2.4.5 but their latest is ^3.2.0, so maybe an upgrade might fix it for you? Who knows.)

@ilyavolodin
Copy link
Member

@platinumazure You might be right. I just tested it again and noticed that I am now getting actual/expected. I rarely run mocha directly, and mostly use it from inside the IDE, so maybe it wasn't happening for a while, but I know last time this issue came up, I wasn't getting them.

@kentcdodds
Copy link
Contributor Author

I'm going to close this in favor of jestjs/jest#3173

Thanks friends!

@kentcdodds
Copy link
Contributor Author

This has been merged so the next version of Jest will support the assert module with much better messaging. This is fantastic 🎉

@vitorbal
Copy link
Member

@kentcdodds Thanks for the update! Great job on that PR :)

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

No branches or pull requests

7 participants