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.

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

Run rust tests from wasi-testsuite #2484

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

zoraaver
Copy link
Contributor

No description provided.

@zoraaver
Copy link
Contributor Author

zoraaver commented Aug 21, 2023

Currently can't merge this due to a failing test on android, ubuntu 22.04:

fd_filestat_get

I can reproduce it locally on OSX but I'm not completely sure I understand if the test is correct.

Also the following rust tests are failing on nightly (can't reproduce locally):

interesting_paths
close_preopen.

Will need to investigate more to figure out why these are failing.

@loganek
Copy link
Collaborator

loganek commented Aug 22, 2023

Please see the related conversation in WebAssembly/wasi-testsuite#59. Depending on the result of that, we'll either delete the test or update the specification (and WAMR implementation accordingly).

@zoraaver zoraaver force-pushed the wasi-rust-tests branch 2 times, most recently from 70dd8af to 2e43ac4 Compare September 11, 2023 17:09
@zoraaver
Copy link
Contributor Author

I've raised a PR in wasi-testsuite to fix the tests which had various permissions issues etc.

@zoraaver zoraaver force-pushed the wasi-rust-tests branch 6 times, most recently from 3805022 to a3dd180 Compare September 19, 2023 18:30
Copy link
Collaborator

@yamt yamt left a comment

Choose a reason for hiding this comment

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

posix.c change seems too big to sneak within a "Run rust tests from wasi-testsuite" PR.
i'd suggest to make it a separate PR.

@zoraaver
Copy link
Contributor Author

zoraaver commented Sep 21, 2023

posix.c change seems too big to sneak within a "Run rust tests from wasi-testsuite" PR. i'd suggest to make it a separate PR.

Hey, thanks for the suggestion. I've made the minimum required changes to get the additional rust WASI tests to pass. Without those changes, the PR will not pass in CI so it won't be possible to split the posix.c changes into a separate PR.

Since some of the tests have changed the expected behavior (e.g. the close_preopen test previously expected closing preopens to fail but the latest version of the test expects it to succeed) it's also not possible to change the posix.c implementation without updating the wasi test commit.

The changes mainly concern behaviour around preopens but please let me know if the motivation behind any of the other changes is unclear.

edit: sorry I forgot we weren't running the rust tests at all before this change so we can split out the posix.c changes into a separate PR. I've raised

#2578
#2579
#2580
#2581

for the posix.c changes. Hopefully it will be easier to review.

After those are merged and this PR is rebased, it should be green and good to merge.

@yamt
Copy link
Collaborator

yamt commented Sep 23, 2023

failing wasi-threads tests should pass if you rebase. cf. #2568

@zoraaver zoraaver requested a review from wenyongh October 6, 2023 09:04
@zoraaver
Copy link
Contributor Author

zoraaver commented Oct 6, 2023

@wenyongh would you mind taking another look? Build is green now so should be good to merge, thanks very much.

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh wenyongh merged commit 74acfd9 into bytecodealliance:main Oct 6, 2023
368 checks passed
@zoraaver zoraaver deleted the wasi-rust-tests branch October 6, 2023 13:49
wenyongh added a commit to wenyongh/wasm-micro-runtime that referenced this pull request Oct 7, 2023
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants