GeezerVison

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

GeezerVison is showing github.com — test: add additional deprecation warning tests for rmdir recursive by iansu · Pull Request #35683 · nodejs/node

Conversation

iansu commented Oct 16, 2020 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

The recently added code coverage in #35653 highlighted an uncovered case of the rmdir recursive deprecation warning. This PR adds two new tests to ensure we cover all the places where the deprecation warning can be shown in both the sync and async versions.

cc @bcoe @nodejs/tooling

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

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 added the test Issues and PRs related to Node.js core tests and test infrastructure. label

Oct 16, 2020


          test: add additional deprecation warning tests for rmdir recursive

4ea6e4a

iansu changed the title ~~fs: add additional deprecation warning tests for rmdir recursive~~ test: add additional deprecation warning tests for rmdir recursive

Oct 16, 2020

iansu force-pushed the more-rmdir-recursive-tests branch from 1c6e32d to 4ea6e4a Compare

October 16, 2020 18:11

bcoe reviewed

Oct 17, 2020

View reviewed changes

bcoe 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.

This is looking good to me, it would be good to run your new tests with coverage.

I will add a section to the docs on how to do so.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

test/parallel/test-fs-rmdir-recursive-warns-not-found.js


		tmpdir.refresh();

bcoe Oct 17, 2020

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.

nit: we usually try to avoid unrelated whitespace changes.

Sorry, something went wrong.

Uh oh!

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

All reactions

iansu Oct 17, 2020

Copy link

Copy Markdown

Contributor Author

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.

Good to know. I'm pretty sure I accidentally added this extra whitespace in my previous PR so I was just cleaning it up.

Sorry, something went wrong.

Uh oh!

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

bcoe reacted with thumbs up emoji

All reactions

👍 1 reaction

Trott approved these changes

Oct 17, 2020

View reviewed changes

Trott 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

Oct 17, 2020

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

Oct 17, 2020

This comment has been minimized.

nodejs-github-bot commented Oct 17, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

aduh95 reviewed

Oct 19, 2020

View reviewed changes

Comment thread

test/parallel/test-fs-rmdir-recursive-warns-on-file.js Resolved

Uh oh!

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


          Update test/parallel/test-fs-rmdir-recursive-warns-on-file.js

2589f98

Co-authored-by: Antoine du Hamel <[email protected]>

codecov-io commented Oct 19, 2020

Copy link

Copy Markdown

Codecov Report

Merging #35683 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #35683   +/-   ##
=======================================
  Coverage   96.40%   96.40%           
=======================================
  Files         220      220           
  Lines       73681    73675    -6     
=======================================
- Hits        71031    71028    -3     
+ Misses       2650     2647    -3

Impacted Files	Coverage Δ
lib/internal/url.js	`93.94% <0.00%> (-0.03%)`	⬇️
lib/_http_server.js	`98.45% <0.00%> (+0.10%)`	⬆️
lib/internal/fs/utils.js	`95.97% <0.00%> (+0.23%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f25fe8...2589f98. Read the comment docs.

All reactions

Sorry, something went wrong.

Uh oh!

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

bcoe approved these changes

Oct 20, 2020

View reviewed changes

nodejs-github-bot commented Oct 20, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot commented Oct 21, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

bcoe commented Oct 21, 2020

Copy link

Copy Markdown

Contributor

@tniessen @aduh95 checking whether your review has been addressed?

All reactions

Sorry, something went wrong.

Uh oh!

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

aduh95 approved these changes

Oct 21, 2020

View reviewed changes

bcoe added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Oct 23, 2020

aduh95 pushed a commit that referenced this pull request

Oct 24, 2020


          test: add additional deprecation warning tests for rmdir recursive

c5b9b5b

PR-URL: #35683
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

aduh95 commented Oct 24, 2020

Copy link

Copy Markdown

Contributor

Landed in c5b9b5b

All reactions

Sorry, something went wrong.

Uh oh!

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

aduh95 closed this

Oct 24, 2020

targos pushed a commit that referenced this pull request

Nov 3, 2020


          test: add additional deprecation warning tests for rmdir recursive

0c5e8ed

PR-URL: #35683
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

targos mentioned this pull request

Nov 3, 2020

v15.1.0 release proposal #35948

Merged

BethGriggs added the dont-land-on-v14.x label

Dec 7, 2020

Reviewers

tniessen

tniessen left review comments

Trott

Trott approved these changes

aduh95

aduh95 approved these changes

+1 more reviewer

bcoe

bcoe 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

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

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.

8 participants

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.

GeezerVison

Conversation

iansu commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

bcoe Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

iansu Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Oct 17, 2020

Uh oh!

Uh oh!

codecov-io commented Oct 19, 2020

Codecov Report

Uh oh!

nodejs-github-bot commented Oct 20, 2020

Uh oh!

nodejs-github-bot commented Oct 21, 2020

Uh oh!

bcoe commented Oct 21, 2020

Uh oh!

aduh95 commented Oct 24, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

iansu commented Oct 16, 2020 •

edited

Loading

Uh oh!

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