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: increase readfile abort coverage by shootermv · Pull Request #36429 · nodejs/node

Conversation

@shootermv

@shootermv shootermv commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Dec 7, 2020
@benjamingr

benjamingr commented Dec 7, 2020

Copy link
Copy Markdown
Member

Hey thanks for this. Please run make lint and fix the linting error :] You can also see the linter output here https://github.com/nodejs/node/pull/36429/files
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr reviewed Dec 7, 2020
View reviewed changes
Comment thread test/parallel/test-child-process-execfile.js Outdated

@benjamingr benjamingr Dec 7, 2020

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.

Suggested change
const callback = () => {};
const callback = common.mustNotCall();

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
benjamingr
benjamingr reviewed Dec 7, 2020
View reviewed changes
Comment thread test/parallel/test-child-process-execfile.js Outdated

@benjamingr benjamingr Dec 7, 2020

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.

Please keep the capitalization: I'd do something like:

Suggested change
// Verify that the if something different than Abortcontroller.signal is passed - ERR_INVALID_ARG_TYPE is thrown
// Verify that passing a non-signal to execFile throws an ERR_INVALID_ARG_TYPE

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@shootermv
shootermv force-pushed the add-readfile-test branch from b415c55 to 2d1c82b Compare December 7, 2020 15:54
@shootermv shootermv changed the title test: incorrect param should throw TypeError test: increase readfile abort converage Dec 7, 2020
@shootermv shootermv changed the title test: increase readfile abort converage test: increase readfile abort coverage Dec 7, 2020
@shootermv
shootermv force-pushed the add-readfile-test branch 2 times, most recently from b4b387e to 29898e6 Compare December 7, 2020 17:23
benjamingr
benjamingr approved these changes Dec 7, 2020
View reviewed changes

@benjamingr benjamingr 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.

Thanks! 🙏

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 7, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 7, 2020
@nodejs-github-bot

nodejs-github-bot commented Dec 7, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Dec 9, 2020
Trott
Trott reviewed Dec 10, 2020
View reviewed changes
Comment thread test/parallel/test-child-process-execfile.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Dec 10, 2020
View reviewed changes
@Trott

Trott commented Dec 10, 2020

Copy link
Copy Markdown
Member

The commit message says readFile but it should be execFile I think?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shootermv @Trott
test: increase execFile abort coverage
5dd58d0 
Verify that if something different than Abortcontroller.signal is passed
to child_process.execFile(), ERR_INVALID_ARG_TYPE is thrown.

PR-URL: nodejs#36429
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Trott force-pushed the add-readfile-test branch from d78ca5b to 5dd58d0 Compare December 10, 2020 13:38
@Trott
Trott merged commit 5dd58d0 into nodejs:master Dec 10, 2020
@Trott

Trott commented Dec 10, 2020

Copy link
Copy Markdown
Member

Landed in 5dd58d0.

Thanks for the contribution! 🎉
benjamingr reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request Dec 21, 2020
@shootermv @targos
test: increase execFile abort coverage
dd705ad 
Verify that if something different than Abortcontroller.signal is passed
to child_process.execFile(), ERR_INVALID_ARG_TYPE is thrown.

PR-URL: #36429
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Dec 22, 2020
v15.5.0 release proposal #36597
Merged
@targos targos added the backport-open-v14.x label Apr 24, 2021
@targos targos mentioned this pull request Apr 24, 2021
[v14.x] Backport AbortController and friends #38386
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@shootermv @targos
test: increase execFile abort coverage
6c86e68 
Verify that if something different than Abortcontroller.signal is passed
to child_process.execFile(), ERR_INVALID_ARG_TYPE is thrown.

PR-URL: nodejs#36429
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 30, 2021
@shootermv @targos
test: increase execFile abort coverage
dc63ca6 
Verify that if something different than Abortcontroller.signal is passed
to child_process.execFile(), ERR_INVALID_ARG_TYPE is thrown.

PR-URL: #36429
Backport-PR-URL: #38386
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 30, 2021
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
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
@benjamingr benjamingr benjamingr approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

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.

5 participants

@shootermv @benjamingr @nodejs-github-bot @Trott @targos
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.