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

Nuke `should` #3035 #3042

Merged
merged 2 commits into from Oct 6, 2017
Merged

Nuke `should` #3035 #3042

merged 2 commits into from Oct 6, 2017

Conversation

@ngeor
Copy link
Contributor

ngeor commented Oct 2, 2017

Description of the Change

The should library has been removed. All tests that were using it are now using expectJS.

Note that I also removed completely the diff integration test, because it was testing the output format provided by should.

Benefits

One less package to worry about.

Applicable issues

  • This PR solves issue 3035
  • It shouldn't impact production code as this removes a dev dependency and only tests have been modified
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 2, 2017

Coverage Status

Changes Unknown when pulling 8fbefc0 on ngeor:nuke-should-3035 into ** on mochajs:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 2, 2017

Coverage Status

Changes Unknown when pulling 8fbefc0 on ngeor:nuke-should-3035 into ** on mochajs:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 2, 2017

Coverage Status

Changes Unknown when pulling 7d851c3 on ngeor:nuke-should-3035 into ** on mochajs:master**.

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Oct 2, 2017

awesome

@ScottFreeCode

This comment has been minimized.

Copy link
Contributor

ScottFreeCode commented Oct 2, 2017

Haven't reviewed all the changes in depth, but want to say thank you for taking this on!!

I just discovered that expect.js doesn't provide the diffs for its strict-equality comparison (equal/be) but does provide them for its deep-equality comparison (eql, no idea why not deepEqual). Do the diffing tests pass if equal/be in the assertion(s) that generate the diffs is/are changed to eql?

@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Oct 2, 2017

No problem at all, I was looking for something to help with and this one looked clear enough. By the way I'm more used to chai and it's also eql there, which one could argue it's not straightforward. I'll give it a try tomorrow with changing the assertions to eql, maybe that helps.

Removed should package from package.json.
Removed should from mocha.opts.
Fixed tests.
Added myself to contributors.
Fixed diff fixture to use eql so that the error message produced by expect matches the one that should used to produce.
@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Oct 3, 2017

Good morning @ScottFreeCode , you were correct, switching to eql just worked for the diff tests. I'd be interested in your opinion on why we should keep these tests, given that they depend on an external tool to pass or fail (i.e. what would happen if expect had its completely own weird output format).

I squashed my commits into one. Looking forward to your feedback :-)

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 3, 2017

Coverage Status

Coverage increased (+0.05%) to 89.674% when pulling b279ed9 on ngeor:nuke-should-3035 into d69bf14 on mochajs:master.

@ScottFreeCode

This comment has been minimized.

Copy link
Contributor

ScottFreeCode commented Oct 3, 2017

Good morning @ScottFreeCode , you were correct, switching to eql just worked for the diff tests.

🎉

I'd be interested in your opinion on why we should keep these tests, given that they depend on an external tool to pass or fail (i.e. what would happen if expect had its completely own weird output format).

The expected vs. actual format isn't specific to expect.js or should, it's common to many assertion libraries (I'm not sure if it's a standard, but it's at least a convention). So they don't really depend on one particular assertion library, just an assertion library with that common behavior.

If we don't already have unit tests that do so, we might be better off creating errors and assigning the expected and actual values to them ourselves and then throwing them or passing them into the diffing stuff, instead of using an assertion library (it would be more... "unit-ey", for lack of a better term, and less "integration-ey"), but that doesn't need to happen at this point.

I squashed my commits into one. Looking forward to your feedback :-)

Will give it a look soon! And thanks again!

Copy link
Contributor

ScottFreeCode left a comment

While there's a lot of code, it looks like it's almost entirely a straightforward switch!

There's a couple very minor things to follow up on, but no reason not to merge this that I can see.

expect(runner).to.have.property('testResults');
expect(runner.testResults).to.have.property('failures');
expect(runner.testResults.failures).to.be.an('array');
expect(runner.testResults.failures.length).to.equal(1);

This comment has been minimized.

Copy link
@ScottFreeCode

ScottFreeCode Oct 4, 2017

Contributor

Nit: whereas should has .have.a.lengthOf, the equivalent in expect is .have.length, which yields better error messages than equal... Would merge this either way, but figure it's worth pointing out.

failure.should.have.properties('err');
expect(failure).to.have.property('title', testTitle);
expect(failure.err.message).to.equal(error.message);
expect(failure).to.have.property('err');

This comment has been minimized.

Copy link
@ScottFreeCode

ScottFreeCode Oct 4, 2017

Contributor

Wow, weird that we've got this test that's asserting the existence of a property only after using said property such that it would get a null reference exception if it didn't exist.

Let's not fix it in this PR, just keep this to should->expect updates. But we may want to drop in a follow-up commit to fix it.

expect(runner).to.have.property('testResults');
expect(runner.testResults).to.have.property('pending');
expect(runner.testResults.pending).to.be.an('array');
expect(runner.testResults.pending.length).to.equal(1);

This comment has been minimized.

Copy link
@ScottFreeCode

ScottFreeCode Oct 4, 2017

Contributor

Another instance of the length nit discussed above.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 6, 2017

Coverage Status

Coverage increased (+0.05%) to 89.674% when pulling ce99bf5 on ngeor:nuke-should-3035 into d69bf14 on mochajs:master.

@ScottFreeCode ScottFreeCode merged commit dcce850 into mochajs:master Oct 6, 2017
4 of 5 checks passed
4 of 5 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 89.674%
Details
licence/cla Contributor License Agreement is signed.
Details
security/snyk No new issues
Details
@ScottFreeCode ScottFreeCode mentioned this pull request Oct 6, 2017
sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019
Removing should library and using expect instead (issue mochajs#3035).
Removed should package from package.json.
Removed should from mocha.opts.
Fixed tests.
Added myself to contributors.
Fixed diff fixture to use eql so that the error message produced by expect matches the one that should used to produce.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.