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: fs.openSync doesn't recive callback #44831

Closed
wants to merge 1 commit into from
Closed

test: fs.openSync doesn't recive callback #44831

wants to merge 1 commit into from

Conversation

wafuwafu13
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 30, 2022
@wafuwafu13 wafuwafu13 changed the title fix(test/fs): fs.openSync doesn't recive callback fs(test): fs.openSync doesn't recive callback Sep 30, 2022
@wafuwafu13 wafuwafu13 changed the title fs(test): fs.openSync doesn't recive callback test: fs.openSync doesn't recive callback Sep 30, 2022
Trott
Trott requested changes Sep 30, 2022
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

The callback is being used to confirm that it is not invoked by openSync().

test/parallel/test-fs-open.js
@@ -82,7 +82,7 @@ for (const extra of [[], ['r'], ['r', 0], ['r', 0, 'bad callback']]) {
}
);
assert.throws(
() => fs.openSync(i, 'r', common.mustNotCall()),
() => fs.openSync(i, 'r'),
Copy link
Member

Choose a reason for hiding this comment

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

common.mustNotCall() creates a function that throws if invoked. So having it there is a way of testing that openSync() does not invoke a callback. Removing it makes the test less effective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. close it.

@wafuwafu13 wafuwafu13 closed this Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott requested changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wafuwafu13 @Trott @joyeecheung @nodejs-github-bot