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: fix some wasi_testsuite cases #6343

Merged
merged 2 commits into from
May 5, 2023
Merged

test: fix some wasi_testsuite cases #6343

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80a62e9
test: clean up `wasi_testsuite` test files
abrown May 4, 2023
8bb6085
test: fix how `wasi_testsuite` maps directories
abrown May 4, 2023
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
54 changes: 34 additions & 20 deletions tests/all/wasi_testsuite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,7 @@ fn wasi_testsuite() -> Result<()> {
const WASI_COMMON_IGNORE_LIST: &[&str] = &[
"environ_get-multiple-variables.wasm",
"environ_sizes_get-multiple-variables.wasm",
"fdopendir-with-access.wasm",
"fopen-with-access.wasm",
"lseek.wasm",
"pread-with-access.wasm",
"pwrite-with-access.wasm",
"stat-dev-ino.wasm",
"close_preopen.wasm",
"dangling_fd.wasm",
"dangling_symlink.wasm",
"directory_seek.wasm",
"fd_advise.wasm",
"fd_filestat_set.wasm",
"fd_flags_set.wasm",
"fd_readdir.wasm",
"interesting_paths.wasm",
];
run_all(
"tests/wasi_testsuite/wasi-common",
Expand All @@ -68,7 +54,12 @@ fn wasi_testsuite() -> Result<()> {
}

fn run_all(testsuite_dir: &str, extra_flags: &[&str], ignore: &[&str]) -> Result<()> {
for module in list_modules(testsuite_dir) {
// In case the previous run ended in failure, we clean up any created files
// that would otherwise be cleaned up at the end of this function.
clean_garbage(testsuite_dir)?;

// Execute and check each WebAssembly test case.
for module in list_files(testsuite_dir, is_wasm) {
if should_ignore(&module, ignore) {
println!("Ignoring {}", module.display());
} else {
Expand All @@ -88,14 +79,19 @@ fn run_all(testsuite_dir: &str, extra_flags: &[&str], ignore: &[&str]) -> Result
}
}
}
Ok(())

// Clean up any created files to avoid making the Git repository dirty.
clean_garbage(testsuite_dir)
}

fn list_modules(testsuite_dir: &str) -> impl Iterator<Item = PathBuf> {
fn list_files<F>(testsuite_dir: &str, filter: F) -> impl Iterator<Item = PathBuf>
where
F: FnMut(&DirEntry) -> bool,
{
WalkDir::new(testsuite_dir)
.into_iter()
.filter_map(Result::ok)
.filter(is_wasm)
.filter(filter)
.map(|e| e.path().to_path_buf())
}

Expand All @@ -122,8 +118,8 @@ fn build_command<P: AsRef<Path>>(module: P, extra_flags: &[&str], spec: &Spec) -
cmd.args(extra_flags);
if let Some(dirs) = &spec.dirs {
for dir in dirs {
cmd.arg("--dir");
cmd.arg(parent_dir.join(dir));
cmd.arg("--mapdir");
cmd.arg(format!("{}::{}", dir, parent_dir.join(dir).display()));
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing the raw directory in the spec test suite to each test, could the directory perhaps be copied to a temporary location and have the test run against that? That way the temporary directory clean up would removing any extra files and the clean_garbage filtering may not be necessary?

Alternatively though explicitly cleaning out files I think is ok, and if this is how the test suite runs upstream I think it's fine to mirror that as well.

Copy link
Contributor Author

@abrown abrown May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Well, right now I believe the other wasi-testsuite test runner (the Python one) expects to clean up any *.cleanup files so I propose we stick with that for now. I opened WebAssembly/wasi-testsuite#74 to actually write down what a test runner should do and I added a comment there about this. How about we change this logic once we decide what to do there?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me 👍

}
}
cmd.arg(module.as_ref().to_str().unwrap());
Expand All @@ -141,6 +137,24 @@ fn build_command<P: AsRef<Path>>(module: P, extra_flags: &[&str], spec: &Spec) -
Ok(cmd)
}

fn clean_garbage(testsuite_dir: &str) -> Result<()> {
for path in list_files(testsuite_dir, is_garbage) {
println!("Removing {}", path.display());
if path.is_dir() {
fs::remove_dir(path)?;
} else {
fs::remove_file(path)?;
}
}
Ok(())
}

fn is_garbage(entry: &DirEntry) -> bool {
let path = entry.path();
let ext = path.extension().map(OsStr::to_str).flatten();
ext == Some("cleanup")
}

#[derive(Debug, Default, Deserialize)]
struct Spec {
args: Option<Vec<String>>,
Expand Down