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

doc: note assert.throws() pitfall #6029

Closed
wants to merge 1 commit into from
Closed

doc: note assert.throws() pitfall #6029

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 3, 2016

Pull Request check-list

  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

doc assert

Description of change

assert.throws() has a pitfall where you may think you are checking error message text when you are not. Document it.

@Trott Trott added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Apr 3, 2016
@Trott Trott mentioned this pull request Apr 3, 2016
4 tasks
@cjihrig
Copy link
Contributor

cjihrig commented Apr 3, 2016

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Apr 3, 2016

LGTM

@Fishrock123
Copy link
Member

maybe we could just fix the API?

Check if the function accepts 3 args then turn arg 2 into a regex?

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

that would be more likely to break someone and the assertion API is locked. Likely best to improve the documentation and leave it

@Trott
Copy link
Member Author

Trott commented Apr 4, 2016

@Fishrock123 There are several inter-related problems with "fixing" this issue in the API rather than just documenting it. "Fixing" it would be semver-major because it's actually acting as designed so it would be a breaking change for anyone using the second-argument-is-a-string as intended.

Maybe it's not clear from the example I'm giving in the doc, but the issue more typically manifests itself like this:

assert.throws(myFunction, 'I think this is an error message that is being checked, but it is not');

In other words, the problem really isn't that someone sends three arguments with the second one as a string. The typical problem (and the one that struck on a test that @jasnell recently fixed) is when there are just two arguments. If you "fix" the API so that when the second argument is a string then it is the error message to check...then you are breaking things for people who are using the API as intended, which is thus:

assert.throws(myFunction, 'this is a message that will be used by the AssertionError when this assertion fires');

So, even if we "fix" it, at a minimum, we should still document it as in this PR for versions prior to 6.

If we "fix" it, then we're introducing a breaking change into a Locked API. So... ¯_(ツ)_/¯

@Fishrock123
Copy link
Member

eh, just document it then

jasnell pushed a commit that referenced this pull request Apr 4, 2016
PR-URL: #6029
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

Landed in 539cede

@jasnell jasnell closed this Apr 4, 2016
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
PR-URL: #6029
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Apr 5, 2016
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
PR-URL: #6029
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
@Trott Trott deleted the throw branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants