Wasi HTTP runtime test #2201

rylev · 2023-12-22T11:48:10Z

This adds a runtime test for wasi:http/outgoing-handler specifically for version 0.2.0-rc-2023-11-10.

The purpose of this PR is:

Add a runtime test that could replace the integration test that was already testing this interface
Explore the addition of a feature of the test component repository for explicitly adapting test components with a given version of the wasi adapter.

Removing the integration test

I have not yet removed the integration test though I think one can make the argument that we can. The integration test exercises both the outgoing and incoming handlers while this runtime test (like all runtime tests) does not concern itself with triggers and therefore only really exercises the outgoing handler.

To be more precise, the runtime test runner uses whatever http handler is defined in the top-level wit directory which for now happens to be version 0.2.0-rc-2023-10-18. However, this is considered an implementation detail. At some point in the future we will add an explicit testing framework for testing trigger handlers.

I will leave it up to @dicej whether he thinks removing the integration test is fine to do or not. At the very least, I think we can remove most of the code from the integration test since there's no really need to test the outgoing handler in both an integration test and a runtime test.

Handling adapter versions

Since this test is explicitly about ensuring support for a specific version of the adapter, this PR also introduces a way to adapt test components with a specific adapter version.

Previously, the test components were not actually components but rather wasm modules that Spin knows how to convert to components during the loading process using spin_componentize. This PR keeps this status quo for all test components with the exception of the newest one. Now any time a component's name ends in v0.2.0-rc-2023-11-10, we run the module through the appropriate component adapter for that version.

In the future, I think it would be good to always adapt components since we're explicitly not testing Spin's ability to do adaption of modules to components in runtime tests, but this is left to a future change.

http-echo server

Lastly, this PR adds an http-echo server runtime test service written in Python. Any test in the future that wants to exercise outbound http, can use this service.

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>

lann · 2023-12-22T13:58:51Z

This is a bit of a tricky one since outgoing wasi uses pollables. I think we'll certainly want to keep at least some integration tests around proxy-style forwarding of request/response bodies; iirc there were a number of subtle bugs around those during Spin 2.0 development.

lann · 2023-12-22T14:07:26Z

tests/test-components/build.rs

            .join("wasm32-wasi")
            .join("debug")
            .join(format!("{binary_name}.wasm"));

+        let adapter_version = package.split('v').last().and_then(|v| match v {
+            "0.2.0-rc-2023-11-10" => Some("15.0.1"),


Why this mapping rather than naming the adapter with the full rc version?

The boring answer is that the version string is long and makes me sad, and the adapter file name was named after the wasmtime version so I decided to do the mapping. It's an easy change, but I'd like to take a step back and think a bit deeper about how we want to do the automagical adapter step, but I think that can wait for a PR in the near future. How does that sound?

15.0.1 just seems more confusing if you don't already have deep wasmtime knowledge. 🤷

Certainly not a blocker.

Yea I hear you. I can change it to your suggestion. I agree it's less confusing. This might change in the future though.

lann · 2023-12-22T14:13:49Z

tests/test-components/build.rs

@@ -3,23 +3,28 @@ use std::{collections::HashMap, path::PathBuf, process::Command};
 fn main() {
    println!("cargo:rerun-if-changed=components");
    println!("cargo:rerun-if-changed=helper");


println!("cargo:rerun-if-changed=adapters");?

lann · 2023-12-22T14:20:25Z

tests/test-components/build.rs

+            "pub const {const_name}: &str = \"{}\";\n",
+            wasm_path.display()


Suggested change

"pub const {const_name}: &str = \"{}\";\n",

wasm_path.display()

"pub const {const_name}: &str = {wasm_path:?};\n"

🤷

As noted in #2158, using the debug implementation does the wrong thing in Windows.

Are these tests actually going to be runnable in windows? 😬

I guess the service-less ones ought to be...

The Python ones should also be runnable, and if they're not that's a bug. I believe the docker containers should also be runnable on Windows, but that's not something I have too much experience with so we'll have to see.

lul - actually Windows failed because we weren't using the debug output... Switched back.

dicej

I think this is a good exploration of how to make the runtime test framework more flexible, but I'm having trouble seeing the advantages of replacing the existing integration test in this case.

90% of the reason for the integration test is to test the inbound handler -- that's the part that was previously missing and which I added, so that's what I was primarily focused on testing. The outbound part was something I tacked on because it was easy. So I don't want to remove coverage from the inbound side, even temporarily.

I do agree that we should strive to minimize overlap among tests, and I also think the integration test is way heavier than it needs to be -- it shouldn't have to start a whole spin process to test this kind of thing. But I also feel that it has a couple of distinct advantages over the runtime test (inbound coverage and use of ephemeral ports) which I'd rather not lose.

dicej · 2023-12-22T14:50:47Z

tests/test-components/components/wasi-http-v0.2.0-rc-2023-11-10/src/lib.rs

+
+impl Component {
+    fn main() -> Result<(), String> {
+        let url = url::Url::parse("http://localhost:8080").unwrap();


Is there a way we can avoid hard-coding a port here? A virtue of the existing integration test is that it atomically binds to an unused port, which has two benefits:

It won't fail if you already have something listening on 8080 (which is a common port to use for ad-hoc web servers)

It can run concurrently with other tests without conflict

It would probably be robust enough for the test runner to bind a random port, unbind, then pass that port in to both the service and test.

There is also a hack for this which can mostly resolve races where you bind a random port, open a connection to that port, then close the port without closing the connection which puts the socket into TIME_WAIT, then SO_REUSEADDR in the service.

Yes, support for ephemeral ports is added in #2193, but I didn't want to have to reimplement it here. I will add that once #2193 is merged.

rylev · 2023-12-22T15:36:55Z

@dicej makes a ton of since. Looks like we'll want to replace the integration test with another test, but we should wait until we have a better test for the inbound trigger exports. 👍

use of ephemeral ports

As noted in the other comment, support for ephemeral ports was added in #2193 and so we can take advantage of that when that PR lands.

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>

Add wasi-http test component

aad8d96

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>

rylev requested a review from dicej December 22, 2023 11:48

Add runtime test

0392714

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>

rylev force-pushed the wasi-http-runtime-test branch from 46f6e5e to 0392714 Compare December 22, 2023 12:44

lann approved these changes Dec 22, 2023

View reviewed changes

dicej reviewed Dec 22, 2023

View reviewed changes

Small PR feedback

d283347

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>

rylev merged commit f324fb2 into main Dec 22, 2023
11 checks passed

rylev deleted the wasi-http-runtime-test branch December 22, 2023 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wasi HTTP runtime test #2201

Wasi HTTP runtime test #2201

rylev commented Dec 22, 2023

lann commented Dec 22, 2023

lann Dec 22, 2023

rylev Dec 22, 2023

lann Dec 22, 2023

rylev Dec 22, 2023

lann Dec 22, 2023

lann Dec 22, 2023

rylev Dec 22, 2023

lann Dec 22, 2023

lann Dec 22, 2023

rylev Dec 22, 2023

rylev Dec 22, 2023

dicej left a comment

dicej Dec 22, 2023

lann Dec 22, 2023

rylev Dec 22, 2023

rylev commented Dec 22, 2023

		"pub const {const_name}: &str = \"{}\";\n",
		wasm_path.display()

	"pub const {const_name}: &str = \"{}\";\n",
	wasm_path.display()
	"pub const {const_name}: &str = {wasm_path:?};\n"

Wasi HTTP runtime test #2201

Wasi HTTP runtime test #2201

Conversation

rylev commented Dec 22, 2023

Removing the integration test

Handling adapter versions

http-echo server

lann commented Dec 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dicej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylev commented Dec 22, 2023