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

test: replace assertion error functions w regex err msg #12603

Closed
wants to merge 1 commit into from
Closed

test: replace assertion error functions w regex err msg #12603

wants to merge 1 commit into from

Conversation

TheLady
Copy link
Contributor

@TheLady TheLady commented Apr 22, 2017 •
edited by refack
Loading

[updated by @refack]
Error message changes are semver-major, so it should be a good practice to tighten up the tests to check for it.
[end update]

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 22, 2017
@vsemozhetbyt
Copy link
Contributor

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

@refack
Copy link
Contributor

refack commented Apr 22, 2017

I'm missing the motivation?

@vsemozhetbyt vsemozhetbyt added the v8 engine Issues and PRs related to the V8 dependency. label Apr 22, 2017
@addaleax
Copy link
Member

@refack Error message changes are semver-major changes right now, so it should be a good practice to tighten up the tests to check for it.

@addaleax addaleax added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Apr 22, 2017
@refack
Copy link
Contributor

refack commented Apr 22, 2017

@addaleax thank you, added to PR desc
@TheLady motivation should be added to commit message

aqrln
aqrln approved these changes Apr 23, 2017
View reviewed changes
Copy link
Contributor

@aqrln aqrln left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, but the commit message is longer than 50 characters. I'd suggest something like test: check error messages instead of types.

Trott
Trott approved these changes Apr 23, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@jasnell
Copy link
Member

jasnell commented Apr 24, 2017

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

jasnell pushed a commit that referenced this pull request Apr 24, 2017
@TheLady @jasnell
test: replace assertion error check with regex
bcb1d9c 
PR-URL: #12603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 24, 2017

Landed in bcb1d9c

@jasnell jasnell closed this Apr 24, 2017
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@TheLady @evanlucas
test: replace assertion error check with regex
ce111ea 
PR-URL: #12603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@refack refack mentioned this pull request Apr 29, 2017
Closed
2 tasks
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@TheLady @evanlucas
test: replace assertion error check with regex
f31f89a 
PR-URL: #12603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@TheLady @evanlucas
test: replace assertion error check with regex
80ceb04 
PR-URL: #12603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@refack refack mentioned this pull request May 2, 2017
Closed
2 tasks
refack pushed a commit to refack/node that referenced this pull request May 10, 2017
@vieiraarturg @refack
test: use assert regexp in tls no cert test
cfe7b34 
Replace the `assert.throws` second argument from a Type to a `RegExp`
matching the entire error message.
Error message changes are `semver-major`, so we assert their content.

PR-URL: nodejs#12891
Refs: nodejs#12603
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Cai <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request May 16, 2017
@TheLady @gibfahn
test: replace assertion error check with regex
db5ffa2 
PR-URL: #12603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
@TheLady @MylesBorins
test: replace assertion error check with regex
0bd0d52 
PR-URL: #12603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@vieiraarturg
test: use assert regexp in tls no cert test
b005727 
Replace the `assert.throws` second argument from a Type to a `RegExp`
matching the entire error message.
Error message changes are `semver-major`, so we assert their content.

PR-URL: nodejs#12891
Refs: nodejs#12603
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Cai <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
Merged
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@vieiraarturg @gibfahn
test: use assert regexp in tls no cert test
3cd258f 
Replace the `assert.throws` second argument from a Type to a `RegExp`
matching the entire error message.
Error message changes are `semver-major`, so we assert their content.

PR-URL: #12891
Refs: #12603
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Cai <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@vieiraarturg @MylesBorins
test: use assert regexp in tls no cert test
d743783 
Replace the `assert.throws` second argument from a Type to a `RegExp`
matching the entire error message.
Error message changes are `semver-major`, so we assert their content.

PR-URL: #12891
Refs: #12603
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Cai <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@TheLady @andrew749
test: replace assertion error check with regex
d6e0aea 
PR-URL: nodejs/node#12603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@aqrln aqrln aqrln approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@santigimeno santigimeno santigimeno approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.