-
Notifications
You must be signed in to change notification settings - Fork 9
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
Improve tests by adding tiny go test cases #90
base: main
Are you sure you want to change the base?
Conversation
h.bodies = []string{} | ||
h.urls = []string{} | ||
h.methods = []string{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly curious, are the empty slices semantically different than using nil
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, I can use nil
if you prefer. It's probably my own aversion to having null pointers in other languages where they are different.
WASI-http doesn't have a great versioning story currently, so explicitly tying to git commit SHA and | ||
`wit-bindgen` tooling is the best we can do. | ||
|
||
Currently, the `wasi-go` codebase has the following versions: | ||
* `v1` corresponds to [244e068c2d](https://github.com/WebAssembly/wasi-http/tree/244e068c2de43088bda308fcdf51ed2479d885f5) and [`wasmtime` 9.0.0](https://github.com/bytecodealliance/wasmtime/tree/release-9.0.0) | ||
|
||
If you want to generate guest code for your particular language, you will need to use [`wit-bindgen` 0.4.0](https://github.com/bytecodealliance/wit-bindgen/releases/tag/wit-bindgen-cli-0.4.0) | ||
|
||
Here is an example for generating the Golang guest bindings: | ||
|
||
```sh | ||
# Setup | ||
git clone https://github.com/WebAssembly/wasi-http | ||
cd wasi-http | ||
# Sync to the definitions for the v1 | ||
git checkout 244e068 | ||
cd .. | ||
|
||
# Generate | ||
wit-bindgen tiny-go wasi-http/wit -w proxy | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for documenting this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned about introducing such large files in the source tree, do you think we could have them regenerated by the Makefile to avoid checking them in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to make this driven by Makefile, though it will mean that tinygo
needs to be installed in order to run these tests.
wdyt?
I can edit the tests to skip the tests if tinygo
is missing, and update the github action on this repo so that it is installed for the GH action CI/CD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can have TinyGo installed in CI without having to recompile it I think it would be the right trade off 👍
Also improves docs a little, fixes a small bug and improves coverage of tests.