Allows writing lint-friendly tests #297

Merged
merged 7 commits into from Nov 10, 2014

Projects

None yet

7 participants

@joshperry
Contributor

This makes the final assertion checks noop functions so that they can be formatted like function calls. This keeps linters (JSLint/JSHint) off our back for having an inop expression.

Fixes #41

joshperry added some commits Oct 13, 2014
@joshperry joshperry Allows writing lint-friendly tests
This makes the final assertion checks noop functions so that they can be formatted like function calls. This keeps linters (JSLint/JSHint) off our back for having an inop expression.

Completes #41
4b0c0b6
@joshperry joshperry Change test to not trigger argument bug cc1357d
@bajtos
bajtos commented Oct 14, 2014

The solution is much simpler and nicer compared to what I was originally imagining. Great job!

@keithamus
Member

@joshperry this change looks great! Thanks for working on this. Could you please also add some documentation around the methods, so that users can discover this feature (for example the documentation here: https://github.com/prodatakey/chai/blob/noopchainfunc/lib/chai/core/assertions.js#L178-L191)

@logicalparadox logicalparadox and 1 other commented on an outdated diff Oct 14, 2014
lib/chai/assertion.js
@@ -69,6 +69,10 @@ module.exports = function (_chai, util) {
util.addChainableMethod(this.prototype, name, fn, chainingBehavior);
};
+ Assertion.addChainableNoop = function(name, fn) {
+ util.addChainableMethod(this.prototype, name, function() { }, fn);
@logicalparadox
logicalparadox Oct 14, 2014 Member

You should create the NOOP function as a file-wide variable and only reference it here.

@joshperry
joshperry Oct 14, 2014 Contributor

I don't disagree... The only thing I'm wondering is, do we want to document this method for use by plugin authors? If so, perhaps we should just make a more idiomatic function for chainable assert terminators. addChainableAssertion maybe?

@keithamus
Member

Any progress @joshperry? It'd be sweet to see some added documentation, and @logicalparadox's suggestion implemented. Then we can look toward merging ๐Ÿ˜„

@joshperry
Contributor

I am out of the country on vacation, I should be back somewhere that I can wrap this up by the middle of next week. Cheers!

@keithamus
Member

Apologies @joshperry. Enjoy your vacation!

@joshperry
Contributor

Was just writing up some documentation and wondered what you guys thought about having an optional message parameter on these functions like its siblings have (e.g. equal, above, etc...).

expect('everthing').to.be.ok();
expect(everything).to.be.ok('Everything was not ok');
@bajtos
bajtos commented Nov 4, 2014

As far as I am concerned, I can already specify the message via expect(what, message), so I don't need the other option. Although I can image the feature may be useful for should users.

However, I would appreciate so much if this PR got landed as soon as possible, and extra bells and whistles were left for later, for a different PR.

My two cents.

@joshperry
Contributor

@bajtos Agreed.

This wouldn't really work with the current assertion execution setup anyway. The assertion is thrown on property access and the function never gets called to set the message. If we wanted to provide a message parameter, there would need to be a substantial refactor of these assertions.

@logicalparadox
Member

@joshperry In addition to the documentation can you also write a snip for the ReleaseNotes that explains the impact of this change. You can see a previous example here. You can just post it as a comment here and I will make sure it makes it into the release notes when I publish. Thanks!

Furthermore, FYI, this will be tagged as release v1.10.0

@joshperry
Contributor

Added some docs... Seems repetitive, but wasn't sure what else to put.

@logicalparadox Definitely

@joshperry
Contributor

No Op Function for Terminating Assertion Properties

The below list of assertions are property getters that assert immediately on access. Because of that, they were written to be used by terminating the assertion chain with a property access.

expect(true).to.be.true;
foo.should.be.ok;

This syntax is definitely aesthetically pleasing but, if you are linting your test code, your linter will complain with an error something like "Expected an assignment or function call and instead saw an expression." Since the linter doesn't know about the property getter it assumes this line has no side-effects, and throws a warning in case you made a mistake.

Squelching these errors is not a good solution as test code is getting to be just as important as, if not more than, production code. Catching syntactical errors in tests using static analysis is a great tool to help make sure that your tests are well-defined and free of typos.

A better option was to provide a function-call form for these assertions so that the code's intent is more clear and the linters stop complaining about something looking off. This form is added in addition to the existing property access form and does not impact existing test code.

expect(true).to.be.true();
foo.should.be.ok();

These forms can also be mixed in any way, these are all functionally identical:

expect(true).to.be.true.and.not.false();
expect(true).to.be.true().and.not.false;
expect(true).to.be.true.and.not.false;

The following assertions can now also be used in the function-call form:

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

Plugin Authors

If you would like to provide this function-call form for your terminating assertion properties, there is a new function to register these types of asserts. Instead of using addProperty to register terminating assertions, simply use addChainableNoop instead; the arguments to both are identical. The latter will make the assertion available in both the attribute and function-call forms and should have no impact on existing users of your plugin.

@joshperry joshperry referenced this pull request in domenic/sinon-chai Nov 5, 2014
Closed

Add lint friendly assertions #48

@keithamus keithamus commented on an outdated diff Nov 5, 2014
lib/chai/core/assertions.js
@@ -185,12 +185,17 @@ module.exports = function (chai, _) {
* expect(false).to.not.be.ok;
* expect(undefined).to.not.be.ok;
* expect(null).to.not.be.ok;
+ * expect(null).to.not.be.ok;
@keithamus
keithamus Nov 5, 2014 Member

This looks to be a duplicate line.

@keithamus
Member

I have a few points which I'd personally like to see wrapped up on this:

  • The tests assert like .to.be.ok, .to.be.ok() and .to.be.ok().and.be.true but I'd also like to see tests that do .to.be.ok.and.be.true()
  • The documentation says "Can also be used in function form which will satiate certain linter errors." for each of the methods - but I worry about the term 'satiate' for EFL speakers. I'd like to see this reworded as "Can also be used as a function, which prevents some linter errors".

With both of those fixed, plus my above comment about the duplicate line, I'd be very happy with this and would happily merge it in, pending @logicalparadox's veto.

@joshperry
Contributor

Thanks for the input, @keithamus.

Agreed on both points. I added a few unit tests exercising the form in your first point and revised the wording in the docs.

@keithamus
Member

@logicalparadox I'm happy with this, I can merge if you're happy.

@joshperry Thanks for putting the effort in, looks great!

@lo1tuma
Contributor
lo1tuma commented Nov 5, 2014

@joshperry thanks for your work, this is awesome.

@logicalparadox
Member

๐Ÿ‘

@abenhamdine

Kudos !
Can't wait to have this patch.
Is it planned to be in 1.9.3 or in 1.10 ? Have you planned a release date for the next version ?

Thanks for all, folks.

@keithamus keithamus merged commit 828f481 into chaijs:master Nov 10, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@keithamus
Member

๐ŸŽ‰

@keithamus
Member

@abenhamdine to quote @logicalparadox:

Furthermore, FYI, this will be tagged as release v1.10.0

@bajtos
bajtos commented Nov 10, 2014

AWESOME! ๐Ÿ™‡

@abenhamdine

โœŒ๏ธ

@elliotf
elliotf commented Feb 5, 2015

I just found this and wanted to say thank you to @joshperry .. developers on my team have had issues when learning the assertions library and done things like expect(foo).to.exists; (or somesuch) not realizing that exists is not an actual assertion. Tests didn't fail because no assertion was being made. With this change, they'll get an error saying so, as long as they call it as a function!

Thank you!

@johanneswuerbach johanneswuerbach referenced this pull request in ember-cli/ember-cli-mocha Feb 5, 2015
Closed

chaijs 1.10 allows to write lint-friendly tests #35

@johanneswuerbach johanneswuerbach added a commit to johanneswuerbach/ember-mocha that referenced this pull request Feb 5, 2015
@johanneswuerbach johanneswuerbach chaijs 1.10 allows to write lint-friendly tests
More details chaijs/chai#297
d1cba18
@johanneswuerbach johanneswuerbach referenced this pull request in emberjs/ember-mocha Feb 5, 2015
Closed

chaijs 1.10 allows to write lint-friendly tests #13

@johanneswuerbach johanneswuerbach added a commit to johanneswuerbach/ember-mocha that referenced this pull request Feb 5, 2015
@johanneswuerbach johanneswuerbach chaijs 1.10 allows to write lint-friendly tests
More details chaijs/chai#297
2f75240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment