Some improvements to the wasi-http client implementation of write. #6161
Conversation
brendandburns
commented
Apr 5, 2023
brendandburns
commented
- Follow up for Begin implementation of wasi-http #5929
- Adds better implementation for body writing on client requests.
brendandburns
force-pushed
the
wasi_http_next
branch
from
4d24323 to
36d4b9b
Compare
brendandburns
force-pushed
the
wasi_http_next
branch
from
36d4b9b to
e24232f
Compare
|
fwiw, the test failed b/c there was a |
I'd strongly +1 this: I was confused just now watching CI logs and seeing queries to an external website. We definitely should not have dependencies on a third-party service like this in our core CI flows, especially given that we will sometimes have time-sensitive CI builds (e.g. for security issues) that cannot tolerate third-party downtime. |
|
Agreed, this is one of the things i thought was OK for an initial prototype but we should remedy pretty quickly. In addition to having integration tests that run against local webservers, we can also pursue writing tests by abstracting over how hyper forms connections, and connect to hyper services in the same process, skipping syscalls entirely. We would still want some tests to connect on a "real" local network to make sure that code gets exercised as well. |
|
I will update the tests in this PR to run against |
brendandburns
force-pushed
the
wasi_http_next
branch
from
0aa1fc4 to
e380fe0
Compare
pchickey
left a comment
pchickey
left a comment
There was a problem hiding this comment.
It looks like the test is failing at the moment for some reason, but once that is fixed this is good to go.
crates/test-programs/wasi-http-tests/src/bin/outbound_request.rs
Outdated
Show resolved
Hide resolved
| ) -> anyhow::Result<()> { | ||
| let _thread = std::thread::spawn(|| { | ||
| run_server().unwrap(); | ||
| }); |
There was a problem hiding this comment.
This is going to break as soon as we run more than one test in a process, which I hope we will do soon. I'll work on the structural changes to the test suite to work that out soon, but its ok for now
| println!("localhost:3000 /get: {r1:?}"); | ||
| assert_eq!(r1.status, 200); | ||
| let method = findHeader(&r1, "x-wasmtime-test-method".to_string()).unwrap_or("MISSING".to_string()); | ||
| let method = r1.header("x-wasmtime-test-method").unwrap_or(&missing); |
There was a problem hiding this comment.
Tests are the one place in Rust I prefer to write code that panics on failure - after all, that is what assert_eq! is doing. So in this case I would write this as let method = r1.header("x-wasmtime-test-method").unwrap(); rather than mess around with having the alternative value around with the correct type, which is clunky.
There was a problem hiding this comment.
Thanks, I'm still learning idiomatic rust. I will correct this in a follow-on PR.
3 participants
