GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: replace assertion error functions w regex err msg by TheLady · Pull Request #12603 · nodejs/node

Conversation

@TheLady

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

[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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@TheLady
test: replace assertion error functions w regex err msg
66d8ac0
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Apr 22, 2017
cjihrig
cjihrig approved these changes Apr 22, 2017
View reviewed changes
addaleax
addaleax approved these changes Apr 22, 2017
View reviewed changes
@vsemozhetbyt

vsemozhetbyt commented Apr 22, 2017

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@refack

refack commented Apr 22, 2017

Copy link
Copy Markdown
Contributor

I'm missing the motivation?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

addaleax commented Apr 22, 2017

Copy link
Copy Markdown
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.

refack and gibfahn reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
refack approved these changes Apr 22, 2017
View reviewed changes
@refack

refack commented Apr 22, 2017

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aqrln
aqrln approved these changes Apr 23, 2017
View reviewed changes

@aqrln aqrln left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Trott
Trott approved these changes Apr 23, 2017
View reviewed changes

@Trott Trott left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
lpinca
lpinca approved these changes Apr 23, 2017
View reviewed changes
santigimeno
santigimeno approved these changes Apr 24, 2017
View reviewed changes
gibfahn
gibfahn approved these changes Apr 24, 2017
View reviewed changes
@jasnell

jasnell commented Apr 24, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jasnell commented Apr 24, 2017

Copy link
Copy Markdown
Member

Landed in bcb1d9c

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
doc: adding "rules of thumb" #12744
Closed
2 tasks done
@evanlucas evanlucas mentioned this pull request May 1, 2017
v7.10.0 proposal #12775
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
doc: add "Troubleshooting" & "best practices" #12791
Closed
2 tasks done
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 gibfahn added the land-on-v6.x label May 16, 2017
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 Olivier Martin
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
v6.11.0 proposal #13059
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

@Trott Trott Trott approved these changes
@addaleax addaleax addaleax approved these changes
@santigimeno santigimeno santigimeno approved these changes
@lpinca lpinca lpinca approved these changes
@cjihrig cjihrig cjihrig approved these changes
+3 more reviewers
@refack refack refack approved these changes
@aqrln aqrln aqrln approved these changes
@gibfahn gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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 Node.js core tests and test infrastructure. 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.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@TheLady @vsemozhetbyt @refack @addaleax @jasnell @Trott @santigimeno @lpinca @cjihrig @aqrln @gibfahn @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.