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

test: move assert to improve clarity #6428

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 27, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test assert

Description of change

A test is run, and then the relevant assertion is put after a different
test. Place the assertion near the relevant thing it is testing for
clarity.

A test is run, and then the relevant assertion is put after a different
test. Place the assertion near the relevant thing it is testing for
clarity.
@Trott Trott added assert Issues and PRs related to the assert subsystem. test Issues and PRs related to the tests. labels Apr 27, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Apr 27, 2016

LGTM

@mscdex
Copy link
Contributor

mscdex commented Apr 27, 2016

A tad unrelated but wouldn't it be better to just use assert.throws(...) there instead?

@Trott
Copy link
Member Author

Trott commented Apr 27, 2016

@mscdex Yup, probably. I started in on that too but then my "no no no, keep the changeset minimal, do that other stuff some other time" impulse took over.

@Trott
Copy link
Member Author

Trott commented Apr 27, 2016

@mscdex And...it looks like I'll be doing exactly that anyway in the fix for #6416 so this might just get closed anyway...

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

LGTM if you decide to land this.

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label Apr 29, 2016
@addaleax
Copy link
Member

Adding in progress here because there doesn’t seem any point in landing this if #6432 does.

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

Agreed.

@Trott
Copy link
Member Author

Trott commented Apr 29, 2016

#6432 landed so I'm going to close this.

@Trott Trott closed this Apr 29, 2016
@Trott Trott deleted the range branch January 13, 2022 22:43
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. test Issues and PRs related to the tests. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants