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

winch: Adding support for integration tests #5588

Merged
merged 3 commits into from
Jan 19, 2023

Conversation

KevinRizzoTO
Copy link
Contributor

@KevinRizzoTO KevinRizzoTO commented Jan 18, 2023

As discussed on our call, this PR adds support for integration tests to Winch in the style of the Cranelift filetests (for WAT). For information on how these work I've added documentation in the winch/docs folder here. There are still some features missing that we are going to add as we continue the project, like support for sending shared flags through the test configuration. Winch doesn't use these currently so it's been omitted to keep this PR smaller.

I also took this as an opportunity to consolidate some dependencies into the top level Cargo.toml. Some dependencies used by Cranelift were needed for the Winch tests, so I wanted to make sure the version would be consistent and consolidated to a single location.

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should tweak a bit the directory structure of the test files? I'm thinking that we'd want to have multiple files per supported architecture: e.g. filetests/x64/simple.wat and filetests/aarch64/simple.wat, if we don't do this we'd probably end up with files named as simple_x64.wat

winch/filetests/Cargo.toml Outdated Show resolved Hide resolved
winch/filetests/src/lib.rs Outdated Show resolved Hide resolved
winch/filetests/src/lib.rs Outdated Show resolved Hide resolved
winch/test-macros/src/lib.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Jan 18, 2023
@KevinRizzoTO
Copy link
Contributor Author

I'm wondering if we should tweak a bit the directory structure of the test files? I'm thinking that we'd want to have multiple files per supported architecture: e.g. filetests/x64/simple.wat and filetests/aarch64/simple.wat, if we don't do this we'd probably end up with files named as simple_x64.wat

Maybe a glob-type pattern would make more sense here! So look for all .wat files in a nested directory structure and then append each one to the name of the test case.

@saulecabrera
Copy link
Member

Maybe a glob-type pattern would make more sense here! So look for all .wat files in a nested directory structure and then append each one to the name of the test case.

Agreed. Poor wording on my end, but I meant how we handle the directory structure of the filetests; the outcome that I expect is what you described.

@github-actions github-actions bot added the winch Winch issues or pull requests label Jan 18, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @saulecabrera

This issue or pull request has been labeled: "cranelift", "winch"

Thus the following users have been cc'd because of the following labels:

  • saulecabrera: winch

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@KevinRizzoTO KevinRizzoTO force-pushed the winch-golden-tests branch 2 times, most recently from 9931b88 to 0da42be Compare January 18, 2023 21:33
This commit adds two new crates into the Winch workspace:
`filetests` and `test-macros`. The intent is to mimic the
structure of Cranelift `filetests`, but in a simpler way.
This commits adds a high level document to outline how to test Winch
through the `winch-tools` utility. It also updates some inline
documentation which gets propagated to the CLI.
Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

This looks good to me; thanks for the most recent improvements.

@saulecabrera saulecabrera merged commit da03ff4 into bytecodealliance:main Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants