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

Merge test_fs_open_no_permissions with test_fcntl_open. NFC #23231

Merged
merged 7 commits into from
Jan 9, 2025
Merged

Merge test_fs_open_no_permissions with test_fcntl_open. NFC #23231

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
27b88f4
Merge test_fs_open_no_permissions with test_fcntl_open. NFC
hoodmane Dec 19, 2024
a9b4aaf
Address review comments
hoodmane Dec 19, 2024
0b983c2
Update test/test_core.py
hoodmane Dec 19, 2024
2dadfcd
Only skip test on nodefs + windows
hoodmane Jan 6, 2025
c30c7ed
Merge branch 'main' into merge-open-no-permissions-test
hoodmane Jan 8, 2025
809be42
Fix merge
hoodmane Jan 8, 2025
b237c14
Cleanup
hoodmane Jan 9, 2025
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
10 changes: 10 additions & 0 deletions test/fcntl/test_fcntl_open.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,18 @@ void test() {
errno = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It it worth trying to split up this main function while you are here? It looks like at least the last 5 lines could b split into test_creat?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems like this would take a fair amount of effort since almost all of the function is one big loop. Probably the loop body should be hoisted into a function.

}

void test_open_create_no_permissions() {
int res = open("a", O_CREAT, 0);
printf("error: %s\n", strerror(errno));
assert(res >= 0);
struct stat st;
assert(stat("a", &st) == 0);
assert((st.st_mode & 0777) == 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the line that fails on windows? Perhaps we could make it more flexible?

As it stands if we add @no_windows the whole test will no longer run on windows. Alternatively you could do this:

if self.getsetting('NODERAWFS') and WINDOWS:
   self.skipTest(...)

That way only that particular variant will be skipped.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's the one: See https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8728106183896305361/+/u/Emscripten_testsuite__core0_/stdout (line 19 of the original)

Copy link
Member

Choose a reason for hiding this comment

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

But it also fails on nodefs, not just on noderawfs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a bunch of test that have something like if nodefs and WINDOWS: skip.. so we can do that here to maybe?

}

int main() {
setup();
test();
test_open_create_no_permissions();
return 0;
}
21 changes: 0 additions & 21 deletions test/fs/test_fs_open_no_permissions.c
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions test/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5543,8 +5543,12 @@ def test_fcntl(self):
self.add_pre_run("FS.createDataFile('/', 'test', 'abcdef', true, true, false);")
self.do_run_in_out_file_test('fcntl/test_fcntl.c')

@crossplatform
hoodmane marked this conversation as resolved.
Show resolved Hide resolved
@also_with_nodefs_both
def test_fcntl_open(self):
hoodmane marked this conversation as resolved.
Show resolved Hide resolved
nodefs = '-DNODEFS' in self.emcc_args or '-DNODERAWFS' in self.emcc_args
if nodefs and WINDOWS:
self.skipTest('Stat mode behavior does not match on Windows')
self.do_run_in_out_file_test('fcntl/test_fcntl_open.c')

@also_with_wasm_bigint
Expand Down Expand Up @@ -5863,10 +5867,6 @@ def test_fs_rename_on_existing(self):
def test_fs_readdir_ino_matches_stat_ino(self):
self.do_runf('fs/test_fs_readdir_ino_matches_stat_ino.c', 'success')

@also_with_nodefs_both
def test_fs_open_no_permissions(self):
self.do_runf('fs/test_fs_open_no_permissions.c', 'success')

@also_with_nodefs_both
@crossplatform
@no_windows('https://github.com/emscripten-core/emscripten/issues/8882')
Expand Down