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

Change assert module's deprecated methods in testing files #4435

Merged
merged 5 commits into from Oct 30, 2020

Conversation

sujin-park
Copy link
Contributor

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

I removed deprecated methods and tested it. equal, deepEqual methods are deprecated since v.9.9.0. Assert Reference

I used strictEqual, deepStrictEqual instead.

Alternate Designs

deprecated

assert.equal(getTitle(this), undefined);
assert.deepEqual(sock.address().family, 'IPv4');

changed

assert.strictEqual(getTitle(this), undefined);
assert.deepStrictEqual(sock.address().family, 'IPv4');

For now, PR's just changed methods that was deprecated. I wonder if i can change code using unexpected library as shown below. I found this issue about migrating assert to Unexpected assertion library. I wonder if there are still assert left.

assert

assert.strictEqual(getTitle(this), undefined);
assert.deepStrictEqual(sock.address().family, 'IPv4');

unexpected assertion library

expect(getTitle(this), 'to be', undefined);
expect(sock.address().family, 'to equal', 'IPv4');

Why should this be in core?

N/A

Benefits

Users can view the modified version of the function within code.

Possible Drawbacks

N/A

Applicable issues

patch release

@sujin-park sujin-park changed the title Change deprecated methods in assert library Change assert module's deprecated methods in testing files Sep 2, 2020
@sujin-park
Copy link
Contributor Author

I'm working on test cases 😢

@boneskull boneskull marked this pull request as draft September 2, 2020 19:47
@boneskull
Copy link
Member

Please mark this PR as ready when ready. I know there's at least one place in our tests that must use assert.

@boneskull boneskull added qa semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Sep 2, 2020
@sujin-park
Copy link
Contributor Author

Thank you for your feedback.
I modified 2 test files including diff tests. When using strictEqual, deepStrictEqual function instead of strictEqual, deepEqual with node version of 10, tests failed because inline diff outputs twice from AssertionError.

스크린샷 2020-09-10 오후 6 50 15

I modified diffs.spec.js so that only one of the two outputs can be printed. also i changed should not display a diff
test to skip when node version is 10. It should be removed when Node.js 10 LTS is not supported.

Please let me know what needs to be modified. Thanks! I marked this PR.

@sujin-park sujin-park marked this pull request as ready for review September 10, 2020 09:54
Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks. I think maybe better to just detect Node.js v10.x and skip the tests. So revert the changes to test/integration/diffs.spec.js and use the same strategy as in test/integration/no-diff.spec.js. do it in a before() hook within the top-level describe().

it's worth mentioning (in the docs, maybe?) that Mocha's "no diff" behavior does not work if the diff is embedded in the message prop of an Error.

@sujin-park
Copy link
Contributor Author

Thanks. I think maybe better to just detect Node.js v10.x and skip the tests. So revert the changes to test/integration/diffs.spec.js and use the same strategy as in test/integration/no-diff.spec.js. do it in a before() hook within the top-level describe().

I reverted the changes to test/integration/diffs.spec.js and use skip tests in the file as in test/integration/no-diff.spec.js.

it's worth mentioning (in the docs, maybe?) that Mocha's "no diff" behavior does not work if the diff is embedded in the message prop of an Error.

I'm considering mentioning this part of the docs that you told, so can I proceed with it in another PR? please let me know if it's okay to proceed like that. Thank you for your comments.

@coveralls
Copy link

coveralls commented Oct 13, 2020

Coverage Status

Coverage remained the same at 94.078% when pulling 00063b8 on sujin-park:sujin-park/change-method into 31116db on mochajs:master.

Copy link
Member

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

LGTM

Netlify build failure seems not related to these changes. But I didn't find why Netlify build failed yet.

@boneskull
Copy link
Member

lgtm, thanks

@boneskull boneskull merged commit 7e490aa into mochajs:master Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants