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

Add option for no-unused-expressions #2102

Closed
ethanresnick opened this issue Mar 18, 2015 · 24 comments
Closed

Add option for no-unused-expressions #2102

ethanresnick opened this issue Mar 18, 2015 · 24 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion triage An ESLint team member will look at this issue soon

Comments

@ethanresnick
Copy link
Contributor

I'm using Chai's expect syntax for testing, and I'm getting no-unused-expression errors. I understand that this is because expressions like expect(...).to.be.undefined look like property access, even though chai's actually using the property's getter to call a method behind the scenes that causes some side effects.

I did a bit of googling and at least a few other people have encountered this.

I'm not sure what the best solution is (I don't know enough about parsing JS to say) but it seems like one possibility might be an option that would not flag property access chains that start with certain identifiers. So the user could specify "expect" as the option's value and then all these chai calls would be let through.

@michaelficarra
Copy link
Member

Just turn the rule off, at least for your test directory. This is a pretty explicit violation of exactly what this rule is trying to prevent.

@nzakas nzakas added the triage An ESLint team member will look at this issue soon label Mar 18, 2015
@nzakas
Copy link
Member

nzakas commented Mar 18, 2015

@ethanresnick can you please provide a sample code snippet and the exact console output that ESLint is giving you?

On the surface, I'm inclined to agree with @michaelficarra that your best option is to disable the rule in your tests, where it's arguably less useful.

@ethanresnick
Copy link
Contributor Author

Sure!

Here's the code, in the most reduced form:

var expect = require('chai').expect;

describe("the number 4", function() {
  it("should be truthy", function() {
    expect(4).to.be.true;
  });
});

And the console output:

5:4  error  Expected an assignment or function call and instead saw an expression  no-unused-expressions

✖ 1 problem (1 error, 0 warnings)

With the config:

{
  "rules": {
    "strict": 0,
    "curly": 0,
    "indent": [2, 2, {indentSwitchCase: true}],
    "brace-style": [2, "stroustrup", { "allowSingleLine": true }],
    "quotes": 0
  },
  "env": {
    "node": true,
    "mocha": true
  }
}

As far as just turning the expression off goes: yes, that's always an option. But, of course, if there's a way to leave it on to get whatever other utility it provides, while telling it not to flag these usages that are beyond my control, that would be nice :)

@devangnegandhi
Copy link

I am facing this exact same issue. I currently use the eslint-disable option as a workaround. But an option to configure this would be awesome!

@qmmr
Copy link
Contributor

qmmr commented Apr 14, 2015

👍
It looks like chai's expect is the most common exception from this rule as it usually eats expressions.

@nzakas
Copy link
Member

nzakas commented Apr 14, 2015

Yup, we are aware. We still don't have a solution that doesn't involve building-in knowledge of Chai. If we can't find one soon, we are going to just recommend you turn off the rule for tests.

@a-c-m
Copy link

a-c-m commented May 14, 2015

Hitting this same issue. Would love a work around, perhaps to exclude that rule from a pattern e.g. *.spec.js (apologies if this is possible already, i could not work out how).

@michaelficarra
Copy link
Member

@a-c-m it is possible through multiple eslint invocations

@nzakas
Copy link
Member

nzakas commented May 15, 2015

Sorry all, we still don't have a way of resolving this without baking-in knowledge of Chai. As a general principle, we do not put library-specific code in ESLint.

While it's unfortunate and not ideal, it's up to you to turn this off where appropriate. You can do that in a config or using online comments. See our docs for more: http://eslint.org/docs/user-guide/configuring

I'm closing this issue as we don't plan on addressing this.

@nzakas nzakas closed this as completed May 15, 2015
@LFDM
Copy link

LFDM commented Jul 11, 2015

FYI, a small chai plugin which gives you these property getters as functions: https://www.npmjs.com/package/chai-lint

@justin-lau
Copy link

dirty-chai works for me. It also automatically converts plugin assertions.

@StephenGrider
Copy link

@justin-lau dirty-chai addressed this issue fantastically for me, thanks for sharing

@niftylettuce
Copy link

👍 for dirty-chai

npm install --save-dev dirty-chai
var chai = require('chai');
+// Load dirty chai first to hook plugin extensions
+var dirtyChai = require('dirty-chai');
+chai.use(dirtyChai);

jonathanglasmeyer added a commit to jonathanglasmeyer/graphql-sequelize that referenced this issue Mar 12, 2016
There is no way to make eslint shut up about chai's assertion
expressions (`expect(foo).to.be.true`) without disabling this rule.

See eslint/eslint#2102
jonathanglasmeyer added a commit to jonathanglasmeyer/graphql-sequelize that referenced this issue Mar 18, 2016
(1)
.eslintrc: Ignore testing globals

Mocha globals like `it` & `describe` trigger eslint warnings because
they are not imported explicitly. Setting them as `globals` in
.eslintrc fixes that.

(2)
Make eslintrc compatible with chai assertion expressions

There is no way to make eslint shut up about chai's assertion
expressions (`expect(foo).to.be.true`) without disabling this rule.

See eslint/eslint#2102

(3)
Lint resolver.test.js
@screendriver
Copy link

👍 for dirty-chai

@djfm
Copy link

djfm commented Sep 25, 2016

Might be a lot of work for not a lot of gain, but wouldn't a rule that ignores "no-unused-expressions" when "env" is set to "mocha" and only for the few mocha magic properties (ok, undefined, ...) make sense?

@platinumazure
Copy link
Member

Might be a lot of work for not a lot of gain, but wouldn't a rule that ignores "no-unused-expressions" when "env" is set to "mocha" and only for the few mocha magic properties (ok, undefined, ...) make sense?

In core ESLint, no, but you are absolutely welcome to write a plugin rule which emulates no-unused-expressions except in the circumstances you described.

@djfm
Copy link

djfm commented Sep 25, 2016

Ok maybe some day when I'm really bored :) After a little more thought I don't think it's worth it or even conceptually such a good idea, eslint-disable-next-line is not so hard to write and provides more information to the reader.

shahata pushed a commit to wix-incubator/eslint-config-wix that referenced this issue Oct 19, 2016
@AMorgaut
Copy link

AMorgaut commented Nov 30, 2016

@nzakas with all respect, you said:

Sorry all, we still don't have a way of resolving this without baking-in knowledge of Chai. As a general principle, we do not put library-specific code in ESLint.

But actually, ESLint, as JSHint, does support "environment options" such as jasmine, mocha, and qunit, plus protractor, atomtest, and embertest on ESLint side... so I think that adding a chai environment option would not break existing ESLint library approach.

I just wrote a related feature request to the JSHint project

As I described in that feature request, another non 'library related' approach could be to add a expr: 'accessor' sub-option to not raise the warning when we ends an expression with the access to an accessor property(cf ES5 Property Attributes) . I mentioned related use cases like forced singleton instantiation.

This library agnostic option can be interesting but:

  • it would probably be more complicated to support than just supporting the chai use case
  • it could hide expected warnings if we only want it on the chai API (such sub option is probably more legitimate to be activated at line level than at file level)

@mattdell
Copy link

mattdell commented Apr 4, 2017

This eslint plugin seems to work for me: https://www.npmjs.com/package/eslint-plugin-chai-friendly

I agree an environment option, similar to mocha, is the best way forward.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Apr 4, 2017

@AMorgaut

But actually, ESLint, as JSHint, does support "environment options" such as jasmine, mocha, and qunit, plus protractor, atomtest, and embertest on ESLint side... so I think that adding a chai environment option would not break existing ESLint library approach.

Environments in ESLint are used to define global variables, and occasionally set parser options. By design, they cannot change the behavior of rules like no-unused-expressions, so it wouldn't be possible to use an environment in the way that you're suggesting.

Additionally, while ESLint used to add environments for custom libraries, it no longer does this.

As I described in that feature request, another non 'library related' approach could be to add a expr: 'accessor' sub-option to not raise the warning when we ends an expression with the access to an accessor property(cf ES5 Property Attributes) . I mentioned related use cases like forced singleton instantiation.

ESLint is a static analysis tool, so it's not possible to tell whether any given property access triggers a getter. Providing a list of accessor properties via configuration is feasible; #8075 is another option.

@AMorgaut
Copy link

AMorgaut commented Apr 4, 2017

@not-an-aardvark Thank for your feedback

Even if it's a bit frustrating, I've seen in the meantime, and understand, that supporting other environment configs are not exactly the chosen path for either JSHint or ESLint. Probably is it better to get a list of environment lint plugins so they could be co-supported by some of the target platform contributors.

ESLint is a static analysis tool, so it's not possible to tell whether any given property access triggers a getter.

Well, I started to work on AST transforms. As far as I saw (both babylon & Espree (used by ESLint) being based on Acorn), it looks quite possible from a static analysis tool to see when a property is defined via

  • Object.defineProperty(context, "name", {get: func});
  • or context = {get foo: func};
  • or class Context { get foo() {} }

But yes there is a limitation when working in cross file contexts...

Providing a list of accessor properties via configuration is feasible

It is the direction that might be taken on the JSHint side as mentioned on this proposal

@artforlife
Copy link

I am having a similar issue in the form of getting the following error:

'should' is assigned a value but never used.

It comes from the following line:

var should = chai.should()

I have tried dirty-chai to no avail. How could I deal with this error?

ilyakam pushed a commit to ilyakam/gulp-pug-linter that referenced this issue Jun 18, 2017
Fix failing tests by ignoring the ESLint rule `no-unused-expressions`
in both `*.spec.js` files and by using the new NodeJS buffer syntax
as seen on the following sources:

- eslint/eslint#2102
- https://nodejs.org/dist/latest-v6.x/docs/api/buffer.html

Update chai and sinon to versions `2.3.4` and `4.0.2`, respectively.
As a result of updating chai, change wording in unit test to reflect
the new `4.x` API. Namely, from `.to.contain` to `.to.deep.include`.

https://greenkeeper.io/
ilyakam pushed a commit to ilyakam/gulp-pug-linter that referenced this issue Jun 18, 2017
Fix failing tests by ignoring the ESLint rule `no-unused-expressions`
in both `*.spec.js` files and by using the new NodeJS buffer syntax
as seen on the following sources:

- eslint/eslint#2102
- https://nodejs.org/dist/latest-v6.x/docs/api/buffer.html

Update chai and sinon to versions `2.3.4` and `4.0.2`, respectively.
As a result of updating chai, change wording in unit test to reflect
the new `4.x` API. Namely, from `.to.contain` to `.to.deep.include`.

https://greenkeeper.io/
@aaron-goshine
Copy link

@nzakas you are one of the expert amongst us, What are the advantages in chai implementation, to omit the useage of the parentheses ()
-- it seems like it might be causing more problem that it solve..

@michaelficarra
Copy link
Member

@aaron-goshine You're absolutely right. The advantage is no parentheses. The disadvantage is mass confusion and many broken assumptions. Nobody should ever have side-effecting getters, without exception.

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 triage An ESLint team member will look at this issue soon
Projects
None yet
Development

No branches or pull requests