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 — Add regex to text-crypto-random by NFoxley · Pull Request #10020 · nodejs/node

Conversation

@NFoxley

@NFoxley NFoxley commented Dec 1, 2016 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

test: add RegExp as a second argument to assert.throws()

27:5 and 28:5 error assert.throws() should include RegExp as a second argument

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Dec 1, 2016
@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Dec 1, 2016
@mscdex

mscdex commented Dec 1, 2016

Copy link
Copy Markdown
Contributor

Please see the commit message guidelines here.

Also, there is a typo in the commit message.
All reactions

Sorry, something went wrong.

Uh oh!

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

imyller commented Dec 1, 2016

Copy link
Copy Markdown
Member

@NFoxley May I kindly ask you to format the commit message as described in CONTRIBUTING guidelines.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@NFoxley

NFoxley commented Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

Yes, I will reformat.
imyller reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes Dec 5, 2016
View reviewed changes
jasnell
jasnell reviewed Dec 5, 2016
View reviewed changes
Comment thread test/parallel/test-crypto-random.js Outdated

@jasnell jasnell Dec 5, 2016

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.

possibly long line. does this pass make lint?

Sorry, something went wrong.

Uh oh!

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

@Fishrock123 Fishrock123 Dec 6, 2016

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.

To pass make lint this should be wrapped and indented to assert.throws( on the next line. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Fishrock123
Fishrock123 previously requested changes Dec 6, 2016
View reviewed changes

@Fishrock123 Fishrock123 left a comment

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. 

/Users/Jeremiah/Documents/node/test/parallel/test-crypto-random.js
  28:1  error  Line 28 exceeds the maximum line length of 80  max-len

When running make lint

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Fishrock123

Fishrock123 commented Dec 6, 2016

Copy link
Copy Markdown
Contributor

@NFoxley Also if you could git commit --amend and add test : to the beginning of the commit message that would be great!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Dec 9, 2016
@Fishrock123 Fishrock123 added lts-watch-v4.x labels Dec 9, 2016
@NFoxley

NFoxley commented Dec 9, 2016

Copy link
Copy Markdown
Contributor Author

Should all be taken care of, thanks for the feedback everyone.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123 Fishrock123 removed the lts-watch-v4.x label Dec 9, 2016
mhdawson
mhdawson approved these changes Dec 12, 2016
View reviewed changes

@mhdawson mhdawson left a comment

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.

Looks like prior comments have been addressed. LGTM provide CI passes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhdawson

mhdawson commented Dec 12, 2016

Copy link
Copy Markdown
Member

CI run: https://ci.nodejs.org/job/node-test-pull-request/5372/
All reactions

Sorry, something went wrong.

Uh oh!

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

mhdawson commented Dec 13, 2016

Copy link
Copy Markdown
Member

In CI run smartos failure was #10166 so unrelated. Arm failure looks to be machine issue so this should be good to land.

@Fishrock123 can you check if you are happy your changes have been addressed ?
All reactions

Sorry, something went wrong.

Uh oh!

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

mhdawson commented Dec 14, 2016

Copy link
Copy Markdown
Member

@NFoxley looks like the change needs to be rebased since there have been other changes to the file landed. There are changes to the same part of the test so you'll need to look to see how the change in the other commmit related to the change you have here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the test-crypto-random-regex branch from 19c540e to 78e1fa8 Compare December 14, 2016 23:10
@Trott

Trott commented Dec 14, 2016

Copy link
Copy Markdown
Member

@mhdawson @Fishrock123 @NFoxley I did the rebase/conflict resolution and pushed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the test-crypto-random-regex branch from 78e1fa8 to d8e3670 Compare December 14, 2016 23:12
@NFoxley @Trott
test: add regex to text-crypto-random
c834c7f
@Trott
Trott force-pushed the test-crypto-random-regex branch from d8e3670 to c834c7f Compare December 14, 2016 23:15
@Trott

Trott commented Dec 22, 2016

Copy link
Copy Markdown
Member

@Fishrock123 I think your comments have been addressed. If so, can you please update your review so this can land? Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented Dec 22, 2016

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/5524/
All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented Dec 24, 2016

Copy link
Copy Markdown
Member

Only CI failure is a known flaky test that is fixed in another PR that should land today or tomorrow.
All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented Dec 24, 2016

Copy link
Copy Markdown
Member

@Fishrock123 Looks good to you?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott dismissed Fishrock123’s stale review December 28, 2016 05:56

trivial fixes completed

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 28, 2016
@NFoxley @Trott
test: add regex to text-crypto-random
5f55faa 
PR-URL: nodejs#10020
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@Trott

Trott commented Dec 28, 2016

Copy link
Copy Markdown
Member

Landed in 5f55faa.
Thanks for the contribution @NFoxley! 🎉
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott closed this Dec 28, 2016
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
v7.4.0 release proposal #10589
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@NFoxley @evanlucas
test: add regex to text-crypto-random
4715865 
PR-URL: #10020
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@NFoxley @evanlucas
test: add regex to text-crypto-random
dff48af 
PR-URL: #10020
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins added the lts-watch-v4.x label Jan 23, 2017
@MylesBorins

MylesBorins commented Jan 23, 2017

Copy link
Copy Markdown
Contributor

Thoughts regarding a backport?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins removed the lts-watch-v4.x label May 8, 2017
@MylesBorins

MylesBorins commented May 8, 2017

Copy link
Copy Markdown
Contributor

ping
All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented May 8, 2017

Copy link
Copy Markdown
Member

@MylesBorins This PR applies cleanly for me to v6.x-staging.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins pushed a commit that referenced this pull request May 9, 2017
@NFoxley @MylesBorins
test: add regex to text-crypto-random
cd1a7ea 
PR-URL: #10020
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels May 9, 2017
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
v6.11.0 proposal #13059
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@NFoxley @andrew749
test: add regex to text-crypto-random
49470a7 
PR-URL: nodejs/node#10020
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
@cjihrig cjihrig cjihrig approved these changes
+2 more reviewers
@Fishrock123 Fishrock123 Fishrock123 left review comments
@mhdawson mhdawson mhdawson 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. crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to Node.js core tests and test infrastructure.

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.

10 participants

@NFoxley @mscdex @imyller @Fishrock123 @mhdawson @Trott @MylesBorins @jasnell @cjihrig @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.