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.

Sign up for GitHub

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

Jump to bottom

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 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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@boneskull
Copy link
Contributor

boneskull commented Oct 2, 2017 •
edited
Loading

awesome

@ScottFreeCode
Copy link
Contributor

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
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.

@ngeor
Removing should library and using expect instead (issue #3035).
b279ed9 
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
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
Copy link

Coverage Status

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

@ScottFreeCode
Copy link
Contributor

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!

ScottFreeCode
ScottFreeCode approved these changes Oct 4, 2017
View reviewed changes
Copy link
Contributor

@ScottFreeCode ScottFreeCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test/reporters/json.spec.js Outdated
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test/reporters/json.spec.js
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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test/reporters/json.spec.js Outdated
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another instance of the length nit discussed above.

@coveralls
Copy link

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
@ScottFreeCode ScottFreeCode mentioned this pull request Oct 6, 2017
Closed
@boneskull boneskull added this to the v4.1.0 milestone Dec 29, 2017
@boneskull boneskull added semver-patch implementation requires increase of "patch" version number; "bug fixes" qa labels Dec 29, 2017
sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019
@ngeor @ScottFreeCode
Nuke should mochajs#3035 (mochajs#3042)
cc10fff 
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.
@snyk-bot snyk-bot mentioned this pull request Jan 22, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ScottFreeCode ScottFreeCode ScottFreeCode approved these changes

Assignees
No one assigned
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Milestone
v4.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ngeor @coveralls @boneskull @ScottFreeCode