Skip to content
This repository was archived by the owner on Nov 7, 2019. It is now read-only.

Add a test for fd_filestat_set_*#14

Merged
kubkon merged 1 commit intoCraneStation:masterfrom
marmistrz:fd_filestat_set
Aug 9, 2019
Merged

Add a test for fd_filestat_set_*#14
kubkon merged 1 commit intoCraneStation:masterfrom
marmistrz:fd_filestat_set

Conversation

@marmistrz
Copy link
Copy Markdown
Member

The test works for me on Linux with current master wasi-common.

marmistrz added a commit to marmistrz/wasi-common that referenced this pull request Aug 9, 2019
This PR matches CraneStation/wasi-misc-tests#14.
The file was created using `cargo build --release --target=wasm32-wasi`
Copy link
Copy Markdown
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

LGTM! Just some minor nitpicks.

use misc_tests::wasi::{wasi_fd_filestat_set_size, wasi_fd_filestat_set_times, wasi_fd_filestat_get, wasi_path_open};
use std::{env, process};

fn test_file_allocate(dir_fd: libc::__wasi_fd_t) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you mind renaming the test case to for example test_fd_filestat_set? I know it's less than ideal and somewhat cumbersome, but I'd like to keep the tests names collision free so that, in the future, if anybody wants to, they can pull all test cases into one binary. :-)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry for that, that was remains of the old testcase :)

);
assert_eq!(stat.st_size, 100, "file size should be 100");

// Allocate should not modify if less than current size
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, is this a runaway comment from a different testcase perhaps?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup :) sorry for that

stat.st_mtim, new_mtim, "mtim should change"
);
assert_eq!(
stat.st_atim, old_atim, "atim should not change"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you mind swapping new_mtim and old_atim for expected literal values? I find that tests which use variables for testing, sometimes can produce misleading results. :-)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I need to remember the old values of stat.st_atim and stat.st_mtim somewhere, because it's stat is reused between fd_filestat_get calls. I can alternatively clone the whole stat structure but I'm not sure if it's cleaner.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, of course, that's fair enough! :-)

@kubkon
Copy link
Copy Markdown
Member

kubkon commented Aug 9, 2019

I'll merge it now, but could you open a PR adding the binary to wasi-common please? :-)

@kubkon kubkon merged commit dcd44c4 into CraneStation:master Aug 9, 2019
@marmistrz
Copy link
Copy Markdown
Member Author

marmistrz commented Aug 9, 2019

The changes in 09eeca1 didn't change the resulting binary. I expect the CI to pass in CraneStation/wasi-common#56 now, after disabling the test on Windows for the time being.

kubkon pushed a commit to CraneStation/wasi-common that referenced this pull request Aug 9, 2019
This PR matches CraneStation/wasi-misc-tests#14.
The file was created using `cargo build --release --target=wasm32-wasi`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants