Chai: Allow writing JSLint/JSHint friendly tests

[adiroiban]

When using expect() for writing asserting, some of the assertion are using properties as the mean for calling assert.

For example

expect(foo).to.exist

JSLint/JSHint will complain about such code, since it expects an assignment or function call.

Hiding a function call, behind getting a property can be sweet but in tests, it leads to such warnings.

I know that one can use assert calls, or not use JSHint/JSLint or configure JSHint/JSLint to ignore such warnings, but I just want to know what is your feeling about this problem.

[logicalparadox]

JSHint is a set of guidelines, not laws, so it has never really been a concern. IMHO, everyone has their own coding style, which may or may not comply with the default JSLint specs. For example, I prefer to have comma separated lists have the comma start on a new line. JSLint hates this:

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

But, it is my personal preference and I am consistent with it.

My point is, I don't see it as a problem. The goal of chai, specifically the expect and should interfaces, is to make test assertions not only functional, but easy to read and compose by humans. Provided that an Assertion will pass or fail as intended, I am not concerned about what JSHint wants to warn me about.

That is not to say that JSHint/Lint doesn't have it's place, or that it isn't a useful tool. It is that in chai's case, our need for ease of use outweighs being a guideline-abiding citizen.

[adiroiban]

Many thanks for your comments. I agree that linters are not laws.

I will look at fixing this in another place :)

[jessecrossen]

In case anyone else looks at this, the problem can be resolved for jshint by configuring it with the "expr" option. Another issue that comes up is the use of "null" and "instanceof" as property names, which can be resolved by suppressing that particular warning. You can get both by including the following in your code:

/* jshint -W024 */
/* jshint expr:true */

[lo1tuma]

Suppressing the jslint/jshint warning does not solve the problem. An expression statement without an assignment has normally no effect because an expression statement is intended to return a value and not actually doing stuff.
Such workarounds of the behavior of the language are misleading and error-prone.

I really hope this will be changed in chai 2.0.

[domenic]

There's nothing wrong with expression statements with side effects. They may not have been common in ES3, the 1999 version of the language, but in ES5, the 2009 version, they're quite normal, due to the existence of getters.

[lo1tuma]

The problem is you wouldn’t expect that such an expression statement would do anything if you just read the code, you have to know that there is a getter defined.
This confuses people but confusion should be avoided!

[domenic]

What I'm trying to say is that you would expect such things if you've been programming JavaScript after 2009.

[lo1tuma]

No, I wouldn’t expect side effects on a simple getter.

[geekdave]

For anyone looking to suppress these warnings globally, I was able to do so by including these options in my .jshintrc:

{
  "es5": true,
  "expr": true
}

[lo1tuma]

The expr option is not available in jslint. Furthermore it is probably not a good idea to suppress the warning globally. The warning exists for a reason.

[gyllstromk]

I agree completely with @lo1tuma. This warning is broadly useful and shouldn't be disabled, at least not to support an unusual idiom which is only used in tests.

A useful resolution would be to allow these to be functions, e.g.:

expect(value).to.exist();

such would remove the jshint warnings while preserving the expect style.

[lo1tuma]

Beware of libraries that assert on property access

[lindem]

I am actually horrified to see people acting like it's no problem, because it is, for the reasons elaborated in the article @lo1tuma linked to in the last post. This is not a style thing. Please reconsider your decision of not fixing this.

[OliverJAsh]

@domenic Your getters could have side-effects, but for the sake of a few exceptions that _only appear in your Chai tests_, I don’t think it’s sensible to disable the expr warning. It’s not good practice to have side-effects in your getters which is why the warning is useful, and why I would prefer not to disable it.

Will look into Must.js now, but it’s a shame the simple solution suggested by @gyllstromk cannot be re-considered given the argument we are making here.

[OliverJAsh]

Many thanks for your comments. I agree that linters are not laws.

I will look at fixing this in another place :)

@adiroiban

What was your other place?

[ToucheSir]

+1 For also allowing getters to be called as functions.

[adiroiban]

@OliverJAsh This was reported 2 years ago... since then I just slowly re-invented the wheel and wrote my own (sane) assertions. I am not a big fan of assert(object).is().fubar() and I prefer assertIsFUBAR(object)

[taybin]

I agree that this is a ridiculous situation and it is disappointing to see the complaints brushed off.

[bajtos]

:+1:

I am willing to contribute a patch to support both expect(value).to.exist and expect(value).to.exist() flavours.

Before starting my work, I'd like to check with the project maintainers that such patch would be accepted.

@logicalparadox @vesln what's your opinion?

My plan is to use the mechanism in addChainableMethod for attaching assertions to a function and modify addProperty to return a no-op function with assertions instead of this (an instance of Assertion).

[mbrgm]

+1
@logicalparadox, @vesln any thoughts on @bajtos 's suggestion?

Silently failing assertions are a potential source of errors. Therefor chai should at least have the option to avoid them by allowing getters to be called as functions.

[logicalparadox]

Can you provide a list of which assertions you would be changing in this patch?

[bajtos]

Can you provide a list of which assertions you would be changing in this patch?

My original intention was to modify addProperty/defineProperty in lib/chai/utils/addProperty.js and thus change all assertions exposed as a property.

As I looked at the places where that method is called, I realised that would change assertions that should not support function call - e.g. have or not. I believe it is possible to support both types of properties (property as a modificator, property as an assertion), although it may need a bit more of work.

Here is the list of assertions I'd like to change:

• ok
• true
• false
• null
• undefined
• exist
• empty
• arguments
• Arguments

[joshperry]

Crazy that I just found this issue as I searched for a way to squelch these jsHint errors in my IDE, they distract from actual issues in my test code and I don't believe disabling the errors is a proper workaround.

Just because we now have getters in JS does _not_ mean that getters with side-effects is recommended or even idiomatic JS. Getters with side-effects, especially throwing or asserting is a major anti-pattern in most other languages that have supported getters since their inception.

I'm definitely on the side of @bajtos and others that have voiced their concerns with this issue. I'm happy to pitch in and help as well.

Just my $0.010b

[keithamus]

A quick workaround for this:

If each of those properties just return an no-op function, while still operating as a getter, it'll appease both camps. They can still be used as getters (as the assertion is done in a getter) but can be called like functions (as they return an no-op function, the getter does the logic but the function can be called). It would also allow a mixed style like:

// These are all the same:
foo.should.be.ok.and.property('bar').be.ok
foo.should.be.ok().and.property('bar').be.ok
foo.should.be.ok.and.property('bar').be.ok()
foo.should.be.ok().and.property('bar').be.ok()

[bajtos]

@keithamus that's exactly what I was proposing. However, the implementation is not trivial, thus I don't want to invest my time until the project maintainers confirm that they are willing to accept such patch.

cc @logicalparadox

[keithamus]

I guess the main complexity would be that the function returned would have to have all of the same properties as the assertion chain. It could be cheated by setting fn.__proto__ = this._proto__ but I'm not sure how compatible that is.

[logicalparadox]

addChainableMethod should do exactly what you are looking for here. It is unlikely you will have to make any changes to the plugin utilities, just the core assertions.

More than willing to accept a PR for the assertions that @bajtos previously listed, my only requirement be that it be entirely backwards compatible on the assertions side.

The unfortunately side effect of this change is that it wont be backwards compatible on plugin side. Plugins which use overwriteProperty on any of the listed assertions will need to switch their assertions to use overwriteChainableMethod, but that shouldn't be too large a concern.

[joshperry]

Would be happy to pitch in to move this forward, though it looks like you've got it sorted.

[bajtos]

@joshperry if you have time to hack on this, then please go ahead. I don't have much time to work on side projects these days.

[joshperry]

Existing tests are passing and I added tests for the new syntax. Not very familiar with the test-suite, but it looks comprehensive so I hope that rules out any serious regressions.

[joshperry]

Odd, one of the PhantomJS tests seems to be failing, but I'm having a difficult time narrowing it down. All the tests work fine through Karma on Chrome and Safari.

[joshperry]

I have no idea why this is breaking, but the arguments don't seem to flow properly through the chain. This may be exposing an existing bug.

 var args = (function(){ return arguments; })(1,2,3);
 expect(args).to.be.arguments().and.ok;  // This works fine, but I have a feeling `ok` is getting `{}`
 expect(args).to.be.arguments().and.not.empty; // This fails with `expected {} not to be empty`

[joshperry]

Yup, this fails on the master branch:

var args = (function(){ return arguments; })(1,2,3);
expect(args).to.not.be.empty;

[joshperry]

Well, I changed the test not to trigger this bug and the PR for the noop function chain is passing all the tests now.

[mockdeep]

Definitely looking forward to having this. It'll protect us from fat-fingered to.be.ture.

[joshperry]

@mockdeep couldn't agree more. It's unfortunate that authors of popular plugins may be another hurdle: domenic/sinon-chai#48

[mockdeep]

@joshperry Bummer. Well hopefully he'll come around. It's a small change and allows people who like either syntax to do as they please. I'm personally in favor of a syntax that fails loudly instead of silently.

[joshperry]

Hey guys, I don't have any back-compat issues, so I figured I would roll this whole change into a new plugin. It supports everything this patch did, with a couple caveats: requires terminating the chain with the function form, and the length and arguments assertions are broken after you chain a function assert.

With these caveats, it also supports custom error messages for these asserts.

I also added an ensure function that you can use with plugins that have assertion properties (open to suggestions on a better name for the function).

spy.should.have.been.calledOnce.ensure();

Enjoy

https://www.npmjs.org/package/dirty-chai

[joshperry]

Just pushed a new version of dirty-chai that hooks any chai plugins that are 'used' after dirty-chai and also makes their property assertions into chainable method assertions.

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

    var sinonChai = require('sinon-chai'),
    chai.use(sinonChai);

    var spy = sinon.stub();

    // Now all of sinon-chai's assertion properties will be chainable methods
    spy.should.have.been.calledOnce(); // instead of spy.should.have.been.calledOnce;
    spy.should.have.been.calledWithNew();

    // The chainable methods can also still be chained in the property form
    // as long as the chain is terminated in the function form.
    spy.should.have.been.calledOnce.and.alwaysCalledWithNew();

[rcodesmith]

For me, this little plugin allowed me to write the assertions in a lint-friendly way: https://github.com/LFDM/chai-lint

[AMorgaut]

Another approach might be to have a Linters (JSHint/JSLint/ESLint/..):

• supporting the distinction between data properties and accessor properties (cf ES5 Property Attributes)
• and providing an option disabling expression statement warnings for accessor warnings

The thing is that I strongly agree it is highly recommended to use functions / methods instead of accessor properties to process something. The "accessor property" name, as well as the "getter" name are quite explicit that their meaning is to return a value, and they still shouldn't, in common application code, have side effect. JS Getters implementations should be safe methods like is GET in HTTP

Still

JavaScript is a language that can be used in different contexts, and in some of them code readability are much more important than performance, and in which some application code rules can be less relevant. As an example, it can be legitimate to use synchronous APIs in simple generators or tasks. On the same idea, the role of chai, over other test tools, is to make behavior tests code more "real life" like.

Note that unit and behavior test code should be as simple as possible, and should, to me, really benefit from some very different coding rules than application, generator, or task code

ex:

• a dedicated lint configuration file (ex: jshintrc)
• potentially some additional dedicated rules

I would find it perfectly legitimate to have some "unit test", "behavior test", or even "chai" option

Some Linters already provide some environment/context specific options such as:

• nodejs
• couchdb
• jquery
• browser
• in development

see JSLint

By the way JSHint already provides some additionnal TDD/BDD related options such as:

• qunit
• jasmine
• mocha

See JSHint Environment options

And ESLint also add supports for

• protractor
• atomtest
• embertest

See ESLint Environment options

So the best thing to me is to send a pull request to at least JSHint to propose a "chai" option (potentially relaxing the "expr" rules for accessor properties)

[henhal]

For those of using WebStorm or other IntelliJ based editors, what I do is to click on the warning's "inspection profile settings" (for JavaScript validity issues -> Expression statement which is not assignment or call") and uncheck it as a warning for the scope "Tests", but keep it as "Warning" for "Everywhere else".

[bajtos]

After using chai with dirty-chai plugin for some time, I eventually decided to switch to should.js and have never looked back ever since. 👋

[ruslanguns]

For vscoders you can supress the warnings hacking the settings.json by adding an entry for jshint.options:

...
"jshint.options": { "expr": true  }

[crfrolik]

Is there a good reason for chai using this pattern? (side-effects on property getters)
https://github.com/moll/js-must#asserting-on-property-access