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

test: improve coverage of lib/fs.js #38604

Closed
wants to merge 2 commits into from
+38 −0
Closed

test: improve coverage of lib/fs.js #38604

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1478960
test: improve coverage of lib/fs.js
pd4d10 May 8, 2021
a82fb75
Apply suggestions from code review
pd4d10 May 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions test/parallel/test-fs-write-file.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,15 @@ fs.open(filename4, 'w+', common.mustSucceed((fd) => {

process.nextTick(() => controller.abort());
}

{
// Test read-only mode
const filename = join(tmpdir.path, 'test6.txt');
fs.writeFileSync(filename, '');

// TODO: Correct the error type
fs.writeFile(filename, s, { flag: 'r' }, common.expectsError({
code: 'EBADF',
message: 'EBADF: bad file descriptor, write'
Copy link
Member

Choose a reason for hiding this comment

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

Not a change to make in this PR, but I wonder if that error message could be improved. Unless I'm misunderstanding, the problem from the user perspective is mode/permissions and not that the file descriptor is bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Since writeFile(fd) and writeFile(path) share the same util function writeAll, this change may need a refactor.

I'll open an issue to track it and look into it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#38607

Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://man7.org/linux/man-pages/man2/write.2.html:

  • EBADF: fd is not a valid file descriptor or is not open for writing.
  • EPERM: The operation was prevented by a file seal; see fcntl(2).

Considering that the file has been opened in the read-only mode and the program is attempting to write to it, isn't EBADF more appropriate here?

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 guess the point is that it's a writeFile operation in the user perspective, but the current error is about file descriptor, which may causes confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

This function just reflects the error that is returned from uv_fs_write, which again reflects the error the OS returns. I don't think we replace the error codes for the other functions, so doing it for this particular function feels a little odd, don't you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pd4d10 This test is failing for Windows. Could you please update it to expect an EPERM for Windows?

}));
pd4d10 marked this conversation as resolved.
Show resolved Hide resolved
}
32 changes: 32 additions & 0 deletions test/parallel/test-fs-writefile-with-fd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,35 @@ tmpdir.refresh();
}));
}));
}


// Test read-only file descriptor
{
// TODO(pd4d10): https://github.com/nodejs/node/issues/38607
const isWindows = process.platform === 'win32';
const expectedCode = isWindows ? 'EPERM' : 'EBADF';

const file = join(tmpdir.path, 'test.txt');

fs.open(file, 'r', common.mustSucceed((fd) => {
fs.writeFile(fd, 'World', common.expectsError({
code: expectedCode,
message: expectedCode + ': bad file descriptor, write'
}));
}));
pd4d10 marked this conversation as resolved.
Show resolved Hide resolved
}

// Test with an AbortSignal
{
const controller = new AbortController();
const signal = controller.signal;
const file = join(tmpdir.path, 'test.txt');

fs.open(file, 'w', common.mustSucceed((fd) => {
fs.writeFile(fd, 'World', { signal }, common.expectsError({
name: 'AbortError'
}));
}));

controller.abort();
}