Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: remove common.fileExists() #22151

Closed
wants to merge 2 commits into from
Closed

test: remove common.fileExists() #22151

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott 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()
0f2140b 
common.fileExists() can be replaced with fs.existsSync().
@nodejs-github-bot
Copy link
Collaborator

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/442/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 6, 2018
@Trott
Copy link
Member Author

Trott commented Aug 6, 2018

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

fesebuv
fesebuv reviewed Aug 6, 2018
View reviewed changes
Copy link

@fesebuv fesebuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Trott
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/

ChALkeR
ChALkeR approved these changes Aug 6, 2018
View reviewed changes
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if tests pass.

@targos
Copy link
Member

targos commented Aug 6, 2018 •
edited
Loading

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

node/test/common/index.mjs

Line 36 in a4c1cf5

fileExists,

@Trott
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/

Trott added a commit to Trott/io.js that referenced this pull request Aug 8, 2018
@Trott
test: remove common.fileExists()
41ae423 
common.fileExists() can be replaced with fs.existsSync().

PR-URL: nodejs#22151
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@Trott
Copy link
Member Author

Trott commented Aug 8, 2018

Landed in 41ae423

@Trott Trott closed this Aug 8, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 8, 2018 •
edited
Loading

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 mentioned this pull request Aug 8, 2018
Closed
3 tasks
@richardlau
Copy link
Member

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()
1473955 
test-trace-event-promises.js was added to the codebase between the last
CI for nodejs#22151 and it landing.

Refs: nodejs#22151
@richardlau richardlau mentioned this pull request Aug 8, 2018
Closed
2 tasks
@richardlau
Copy link
Member

Fix for the additional test: #22200

MylesBorins pushed a commit that referenced this pull request Aug 8, 2018
@richardlau @MylesBorins
test: remove common.fileExists()
d7e1847 
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@Trott
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 pushed a commit that referenced this pull request Aug 11, 2018
@richardlau @targos
test: remove common.fileExists()
6f47a84 
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
targos pushed a commit that referenced this pull request Aug 11, 2018
@Trott @targos
test: remove common.fileExists()
0385e09 
common.fileExists() can be replaced with fs.existsSync().

PR-URL: #22151
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@rvagg rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@refack
Copy link
Contributor

refack commented Sep 3, 2018 •
edited
Loading

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

@Trott Trott deleted the existsSync branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fesebuv fesebuv fesebuv left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@richardlau richardlau richardlau approved these changes

@tniessen tniessen tniessen approved these changes

@maclover7 maclover7 maclover7 approved these changes

@trivikr trivikr trivikr approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@starkwang starkwang starkwang approved these changes

@fhinkel fhinkel fhinkel approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.