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

assert: fix misformatted error message #11254

Closed
wants to merge 2 commits into from
Closed

assert: fix misformatted error message #11254

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 9, 2017

Before: Missing expected exception..

Afer: Missing expected exception.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

@Trott Trott added assert Issues and PRs related to the assert subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 9, 2017
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Feb 9, 2017
@Trott
Copy link
Member Author

Trott commented Feb 9, 2017

Technically semver-major because of the idiosyncratic way we have to be
super-careful with error messages. If anyone thinks it violates the
Locked API due to that, I won't argue the point. (Paging @ChALkeR!)

My thinking is that this doesn't violate the Locked API because it's a bug fix, even if we have a policy (at least at this time) to treat this particular type of bug fix like it's semver major.

But a "sorry, semver major is semver major, deal with it" approach also makes sense. (In which case, I might see if there's a general sentiment to unlock the API but that's another conversation.)

@gibfahn
Copy link
Member

gibfahn commented Feb 9, 2017

LGTM, and definitely seems related to #11200.

lpinca
lpinca reviewed Feb 10, 2017
View reviewed changes
lib/assert.js Outdated
@@ -342,7 +342,7 @@ function _throws(shouldThrow, block, expected, message) {
actual = _tryBlock(block);

message = (expected && expected.name ? ' (' + expected.name + ').' : '.') +
(message ? ' ' + message : '.');
(message ? ' ' + message : '');
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this end with . when message is truthy?

(message ? ` ${message}.` : '');

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, unless message is expected to already end with ., so ¯\(ツ)/¯. And there should also be a : rather than . after expected if message is truthy.

Thinking about this some more and looking at the uses, I wonder if the thing to do is get rid of all the logic for appending . in the first place. Nothing else appends .. And this mostly only seems to do it wrong!

Current master results in messages like this:

AssertionError: Missing expected exception..
AssertionError: Missing expected exception. foo bar baz

wat?

Compare to assert.strictEqual() which does not append .:

AssertionError: 'a' === 'b'
AssertionError: foo bar baz

Copy link
Member Author

Choose a reason for hiding this comment

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

@lpinca OK, I just pushed some improvements based on your comment. PTAL

Trott added 2 commits February 10, 2017 11:11
@Trott
assert: fix misformatted error message
9df45db 
Before: `Missing expected exception..`

Afer: `Missing expected exception.`
@Trott
squash: improvements
567284f
@lpinca
Copy link
Member

lpinca commented Feb 10, 2017

@Trott

Thinking about this some more and looking at the uses, I wonder if the thing to do is get rid of all the logic for appending . in the first place.

This also makes sense imo, I think most error messages don't use . at all.

@Trott
Copy link
Member Author

Trott commented Feb 10, 2017

I'm going to apply the blocked label to this until #11304 lands or there's some other satisfactory resolution for #11200.

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Feb 10, 2017
@jasnell
Copy link
Member

jasnell commented Feb 11, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6355/

@Trott Trott removed the blocked PRs that are blocked by other issues or PRs. label Feb 15, 2017
@Trott
Copy link
Member Author

Trott commented Feb 15, 2017

assert is now Stable rather than Locked so this is un-blocked.

Trott added a commit to Trott/io.js that referenced this pull request Feb 15, 2017
@Trott
assert: fix misformatted error message
0af4183 
Before: `Missing expected exception..`

Afer: `Missing expected exception.`

PR-URL: nodejs#11254
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@Trott
Copy link
Member Author

Trott commented Feb 15, 2017

Landed in 0af4183

@Trott Trott closed this Feb 15, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@Trott Trott deleted the fix-assert branch January 13, 2022 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Trott @gibfahn @lpinca @jasnell @cjihrig @joyeecheung @nodejs-github-bot