integration tests #121
integration tests #121
Conversation
| for (ready, pollable), waker in zip(zip(ready, pollables), wakers): | ||
| if ready: | ||
| pollable.__exit__() | ||
| pollable.__exit__(None, None, None) |
There was a problem hiding this comment.
This was the issue I introduced by doing the mypy types to match the usual __exit__ pattern... maybe this was a mistake.
|
Ok classic "works on my machine" but not CI... I'll try and dig in. I think I may need to rework some of this |
tests/componentize.rs
Outdated
| Command::new("python3") | ||
| .current_dir(dir.path()) | ||
| .args(["-m", "venv", ".venv"]) | ||
| .assert() | ||
| .success(); | ||
|
|
||
| Command::new("./.venv/bin/pip") | ||
| .current_dir(dir.path()) | ||
| .args(["install", "wasmtime"]) | ||
| .assert() | ||
| .success(); |
There was a problem hiding this comment.
@dicej I'm not sure if you have a preference here or not. This makes sure the tests can run as long as you have Python installed. But we could also have some docs in the contributing section that python + wasmtime should already be setup. We'll have the same issue with mypy for linting.
|
@dicej ok after much wrestling with windows, this should be ready. All example running and linting is now happening in Rust tests, and with that I was able to get much better coverage. |
There was a problem hiding this comment.
Looks great overall. Thanks so much for doing this!
As I mentioned inline, I tend to prefer we use library crates and/or std over shelling out to curl, tar, and nc since it reduces the number of utilities that need to be installed to run the tests. Another advantage is that we would have more precise control for the HTTP and TCP examples: rather than telling e.g. curl to do timed retries while waiting for wasmtime to start listening for connections, we could programmatically start the server on an emphemeral port and then use reqwest to send a request to that port (which also avoids potential port conflicts if we ever have more than one HTTP test running in parallel). The downside is that there are more steps to setting up an HTTP server and hooking it up to wasmtime vs. just running wasmtime serve. Anyway, none of that is a blocker, but something to keep in mind as we add more examples and/or tests.
I do think it's reasonable to shell out to pip and python given that it would presumably be more complicated to replace those with library crates.
| } | ||
|
|
||
| fn install_numpy(path: &Path) { | ||
| Command::new("curl") |
There was a problem hiding this comment.
Maybe we should use e.g. reqwest here instead of relying on curl being installed?
| .assert() | ||
| .success(); | ||
|
|
||
| Command::new("tar") |
There was a problem hiding this comment.
Maybe we should use the tar crate plus flate2 instead of relying on tar here?
| .success() | ||
| .stdout("Component built successfully\n"); | ||
|
|
||
| Command::new("wasmtime") |
There was a problem hiding this comment.
We could use wasmtime as a library here (and elsewhere) instead of requiring the command to be installed. Only the sandbox test needs to use the Python wrapper for wasmtime; the rest could use it as a library.
| And the mome raths outgrabe. | ||
| "; | ||
|
|
||
| Command::new("curl") |
There was a problem hiding this comment.
Same comment about curl vs. reqwest I mentioned above.
| .success() | ||
| .stdout("Component built successfully\n"); | ||
|
|
||
| let mut nc_handle = std::process::Command::new("nc") |
There was a problem hiding this comment.
Could use std::net::TcpStream (or the tokio equivalent) instead of nc here.
| .join(if cfg!(windows) { "Scripts" } else { "bin" }) | ||
| } | ||
|
|
||
| fn install_numpy(path: &Path) { |
There was a problem hiding this comment.
It's slightly annoying we have two copies of this function and venv_path across bindings.rs and componentize.rs, but I'm not sure it's worth consolidating them into their own library crate. Something to revisit if the amount of duplication increases over time.
|
Good points. I was so focused on just getting the readme examples to work, I neglected this (at some point I thought of pulling in wasmtime as a lib, but pressed on). I can do a round two when I get a chance to clean this up and be a little more maintainable. It would also have the benefit of running the |
Moves the
test_examples.shandlint_examples.shto Rust integration tests.This is indeed much nicer and makes it possible to actually test the Http and Tcp ones, for which I introduced a bug in my last changes that was now caught 🤦🏻