★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/22151Nouvelle recherche | Portail wanayoo
Skip to content

/node

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.

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

test: remove common.fileExists() #22151

Closed
wants to merge 2 commits into from

Conversation

Reviewers

@fesebuv fesebuv

@fhinkel fhinkel

@ChALkeR ChALkeR

@jasnell jasnell

@benjamingr benjamingr

@lpinca lpinca

@targos targos

@cjihrig cjihrig

@maclover7 maclover7

@tniessen tniessen

@richardlau richardlau

@BridgeAR BridgeAR

@starkwang starkwang

@trivikr trivikr

Assignees
No one assigned
Labels
test
Projects
None yet
Milestone
No milestone
@Trott
Copy link
Member

Trott commented Aug 6, 2018

common.fileExists() can be replaced with fs.existsSync().

fs.existsSync() was undeprecated in Node.js 6.8.0.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
@Trott
test: remove common.fileExists() 
common.fileExists() can be replaced with fs.existsSync().
Loading status checks…
0f2140b
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the test label Aug 6, 2018

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member Author

Trott commented Aug 6, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16214/
@richardlau

richardlau approved these changes Aug 6, 2018
View changes

@starkwang

starkwang approved these changes Aug 6, 2018
View changes

@maclover7

maclover7 approved these changes Aug 6, 2018
View changes

@targos

targos approved these changes Aug 6, 2018
View changes

@fesebuv

fesebuv reviewed Aug 6, 2018
View changes

Copy link

fesebuv left a comment

Seems good to me, but I would advice to double check why some tests are failing.
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member Author

Trott commented Aug 6, 2018

https://ci.nodejs.org/job/node-test-commit-custom-suites/458/default/console failed because of stale processes. Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16225/
@lpinca

lpinca approved these changes Aug 6, 2018
View changes

@cjihrig

cjihrig approved these changes Aug 6, 2018
View changes

@ChALkeR

ChALkeR approved these changes Aug 6, 2018
View changes

Copy link
Member

ChALkeR left a comment

LGTM if tests pass.
@targos

This comment has been minimized.

Sign in to view
Copy link
Member

targos commented Aug 6, 2018 •
edited

git grep fileExists shows it's still used in test/common/index.mjs.

node/test/common/index.mjs

Line 36 in a4c1cf5

fileExists,

@Trott Trott force-pushed the Trott:existsSync branch from 3cdcba4 to 4d0bc78 Aug 6, 2018

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member Author

Trott commented Aug 6, 2018

CI with @targos's comment addressed: https://ci.nodejs.org/job/node-test-pull-request/16231/
@BridgeAR

BridgeAR approved these changes Aug 6, 2018
View changes

@benjamingr

benjamingr approved these changes Aug 6, 2018
View changes

@jasnell

jasnell approved these changes Aug 6, 2018
View changes

@trivikr

trivikr approved these changes Aug 7, 2018
View changes

@tniessen

tniessen approved these changes Aug 7, 2018
View changes

@fhinkel

fhinkel approved these changes Aug 8, 2018
View changes

Trott added a commit to Trott/io.js that referenced this pull request Aug 8, 2018

@Trott
test: remove common.fileExists() 
common.fileExists() can be replaced with fs.existsSync().

PR-URL: nodejs#22151
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
41ae423
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member Author

Trott commented Aug 8, 2018

Landed in 41ae423

@Trott Trott closed this Aug 8, 2018

@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 8, 2018 •
edited

Strangely, we have CI fails now due to this test:

node/test/parallel/test-trace-event-promises.js

Line 36 in 41ae423

assert(common.fileExists(file));

See #22104 (comment)

@vsemozhetbyt vsemozhetbyt referenced this pull request Aug 8, 2018

Closed

doc: add doc for --loader option #22104

3 of 3 tasks complete
@richardlau

This comment has been minimized.

Sign in to view
Copy link
Member

richardlau commented Aug 8, 2018

Strangely, we have CI fails now due to this test

That test was added to the codebase in between the last CI for this PR (2 days ago) and it landing today.

richardlau added a commit to richardlau/node-1 that referenced this pull request Aug 8, 2018

@richardlau
test: remove common.fileExists() 
test-trace-event-promises.js was added to the codebase between the last
CI for nodejs#22151 and it landing.

Refs: nodejs#22151
1473955

@richardlau richardlau referenced this pull request Aug 8, 2018

Closed

fixup: test: remove common.fileExists() #22200

2 of 2 tasks complete
@richardlau

This comment has been minimized.

Sign in to view
Copy link
Member

richardlau commented Aug 8, 2018

Fix for the additional test: #22200

MylesBorins added a commit that referenced this pull request Aug 8, 2018

@richardlau @MylesBorins
test: remove common.fileExists() 
test-trace-event-promises.js was added to the codebase between the last
CI for #22151 and it landing.

PR-URL: #22200
Refs: #22151
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
Loading status checks…
d7e1847
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member Author

Trott commented Aug 8, 2018

Oops! Add this to the list of reasons to get https://github.com/nodejs/commit-queue happening. @nodejs/commit-queue

targos added a commit that referenced this pull request Aug 11, 2018

@richardlau @targos
test: remove common.fileExists() 
test-trace-event-promises.js was added to the codebase between the last
CI for #22151 and it landing.

PR-URL: #22200
Refs: #22151
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
a6f5e95

targos added a commit that referenced this pull request Aug 11, 2018

@richardlau @targos
test: remove common.fileExists() 
test-trace-event-promises.js was added to the codebase between the last
CI for #22151 and it landing.

PR-URL: #22200
Refs: #22151
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
6f47a84

targos added a commit that referenced this pull request Aug 11, 2018

@Trott @targos
test: remove common.fileExists() 
common.fileExists() can be replaced with fs.existsSync().

PR-URL: #22151
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
0385e09

@rvagg rvagg referenced this pull request Aug 13, 2018

Merged

Release proposal: v10.9.0 #22295

This was referenced Aug 16, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@refack

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Sep 3, 2018 •
edited

Missed one in /internet/:
https://ci.nodejs.org/job/node-test-commit-custom-suites/675/default/testReport/junit/(root)/test/internet_test_trace_events_dns/
New test added in #21840
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.