Extract get_router and add simple test. #29

heisencoder · 2022-11-23T01:30:21Z

This PR extracts the logic from run_server that creates a new Router into a helper function named get_router to make it easier to test without needing to start the full server.

This also adds a simple test that verifies current behavior for returning a br compressed wasm.wasm file.

This change is in preparation for adding support dynamically change the available compression.

See issue #28 for context.

heisencoder · 2022-11-23T01:32:31Z

Hi @jakobhellermann and @daxpedda -- I'm starting with this simple refactoring PR with a simple test, but plan to later add on support for serving gzip compressed files based on the accepting-encoding header. Please let me know if this overall approach looks like a good start with regard to refactoring and testing. Thanks! :)

heisencoder · 2022-11-23T03:23:02Z

src/server.rs

+    let mut address_string = options.address;
+    if !address_string.contains(":") {
+        address_string +=
+            &(":".to_owned() + &pick_port::pick_free_port(1334, 10).unwrap_or(1334).to_string());
+    }
+    let addr: SocketAddr = address_string.parse().expect("Couldn't parse address");
+
+    if options.https {
+        let certificate = rcgen::generate_simple_self_signed([String::from("localhost")])?;
+        let config = RustlsConfig::from_der(
+            vec![certificate.serialize_der()?],
+            certificate.serialize_private_key_der(),
+        )
+        .await?;
+
+        tracing::info!("starting webserver at https://{}", addr);
+        axum_server_dual_protocol::bind_dual_protocol(addr, config)
+            .set_upgrade(true)
+            .serve(app.into_make_service())
+            .await?;
+    } else {
+        tracing::info!("starting webserver at http://{}", addr);
+        axum_server::bind(addr).serve(app.into_make_service()).await?;
+    }
+
+    Ok(())
+}


This code is unchanged as compared to lines 82-107 of the original file.

daxpedda

Just to clarify, I'm not a maintainer of this repository, so no merge rights, but happy to be pinged to review anyway, thanks!

You should probably not ping jakobhellermann, owners of a repo get notified anyway.

daxpedda · 2022-11-23T13:29:57Z

src/server.rs

+
+    /// Headers for requests from 127.0.0.1 and local IP:
+    ///
+    /// In this request, br is missing from the "accept-encoding" header


This is confusing, the test below does also have br in it's accept-encoding header, as far as I can see.

I've deleted this whole comment, particular since this is largely captured in #28. For this PR, I'm leaving the br in the accept-encoding in the test just to verify existing behavior and to have the testing scaffolding in place.

daxpedda · 2022-11-23T13:30:11Z

src/server.rs

+    /// "sec-ch-ua-platform": "\"Linux\"", "accept": "*/*", "sec-fetch-site": "same-origin",
+    /// "sec-fetch-mode": "cors", "sec-fetch-dest": "empty", "referer": "http://127.0.0.1:1334/",
+    /// "accept-encoding": "gzip, deflate, br", "accept-language": "en-US,en;q=0.9"}
+


This is probably not useful as a comment.

daxpedda · 2022-11-23T13:31:57Z

src/server.rs

+            no_module: false,
+        };
+
+        let wasm_file =


Not exactly ideal. I think generally the issue is that wasm-server-runner has no CI, so I'm not entirely sure if adding tests that are not really straightforward to run makes sense.

Very valid point! I've done some rework to this test so that it no longer calls generate and instead just has a dummy br compressed file with everything else in WasmBindgenOutput either empty or with dummy values.

daxpedda · 2022-11-23T13:32:13Z

src/server.rs

+            Path::new("example-project/target/wasm32-unknown-unknown/debug/example-project.wasm");
+        let output = wasm_bindgen::generate(&options, wasm_file).unwrap();
+        let router = get_router(&options, output);
+        println!("{:?}", &router);


Again I'm assuming debugging leftover.

Good catch! Removed

daxpedda · 2022-11-23T13:33:09Z

src/server.rs

+            .await;
+        assert_eq!(res.status(), StatusCode::OK);
+        let result = res.chunk().await.unwrap();
+        assert_ne!(result[0..3], Bytes::from(vec![0x1f, 0x8b, 0x08]));


You could probably just load the file that you have the path to already and just compare the whole thing.

Good idea! I went ahead and just created a short dummy "compressed" file as a Vec and compared against that. Since I'm not really testing the compression itself in this test, I omitted that logic here.

daxpedda · 2022-11-23T13:33:38Z

src/server.rs

+        // Test without br compression
+        // let mut res = client
+        //     .get("/api/wasm.wasm")
+        //     .header("accept-encoding", "gzip, deflate")
+        //     .send()
+        //     .await;
+        // assert_eq!(res.status(), StatusCode::OK);
+        // let result = res.chunk().await.unwrap();
+        // // This is the gzip 3-byte file header
+        // assert_eq!(result[0..3], Bytes::from(vec![0x1f, 0x8b, 0x08]));
+
+        //tokio_test::block_on(server).unwrap();


This is probably not useful to leave here.

heisencoder · 2022-11-23T19:02:43Z

Thank you @daxpedda for all your constructive comments! I should have some time after U.S. Thanksgiving to take another pass.

heisencoder

Thank you @daxpedda for your comments! I've uploaded changes based on these comments as a new commit for easier tracking, although I'm happy to do a squash rebase if that's easier for this repo (in any case, I'm assuming that this would get squash merged).

heisencoder · 2022-11-24T00:51:28Z

src/server.rs

+
+    /// Headers for requests from 127.0.0.1 and local IP:
+    ///
+    /// In this request, br is missing from the "accept-encoding" header


I've deleted this whole comment, particular since this is largely captured in #28. For this PR, I'm leaving the br in the accept-encoding in the test just to verify existing behavior and to have the testing scaffolding in place.

heisencoder · 2022-11-24T00:51:36Z

src/server.rs

+    /// "sec-ch-ua-platform": "\"Linux\"", "accept": "*/*", "sec-fetch-site": "same-origin",
+    /// "sec-fetch-mode": "cors", "sec-fetch-dest": "empty", "referer": "http://127.0.0.1:1334/",
+    /// "accept-encoding": "gzip, deflate, br", "accept-language": "en-US,en;q=0.9"}
+


heisencoder · 2022-11-24T00:55:48Z

src/server.rs

+            no_module: false,
+        };
+
+        let wasm_file =


Very valid point! I've done some rework to this test so that it no longer calls generate and instead just has a dummy br compressed file with everything else in WasmBindgenOutput either empty or with dummy values.

heisencoder · 2022-11-24T00:56:06Z

src/server.rs

+            Path::new("example-project/target/wasm32-unknown-unknown/debug/example-project.wasm");
+        let output = wasm_bindgen::generate(&options, wasm_file).unwrap();
+        let router = get_router(&options, output);
+        println!("{:?}", &router);


Good catch! Removed

heisencoder · 2022-11-24T00:57:49Z

src/server.rs

+            .await;
+        assert_eq!(res.status(), StatusCode::OK);
+        let result = res.chunk().await.unwrap();
+        assert_ne!(result[0..3], Bytes::from(vec![0x1f, 0x8b, 0x08]));


Good idea! I went ahead and just created a short dummy "compressed" file as a Vec and compared against that. Since I'm not really testing the compression itself in this test, I omitted that logic here.

heisencoder · 2022-11-24T00:57:55Z

src/server.rs

+        // Test without br compression
+        // let mut res = client
+        //     .get("/api/wasm.wasm")
+        //     .header("accept-encoding", "gzip, deflate")
+        //     .send()
+        //     .await;
+        // assert_eq!(res.status(), StatusCode::OK);
+        // let result = res.chunk().await.unwrap();
+        // // This is the gzip 3-byte file header
+        // assert_eq!(result[0..3], Bytes::from(vec![0x1f, 0x8b, 0x08]));
+
+        //tokio_test::block_on(server).unwrap();


daxpedda

Otherwise LGTM.

daxpedda · 2022-11-24T11:25:34Z

src/server.rs

+    fn fake_wasm_bindgen_output() -> WasmBindgenOutput {
+        WasmBindgenOutput {
+            js: "fake js".to_string(),
+            compressed_wasm: FAKE_BR_COMPRESSED_WASM.to_vec(),
+            snippets: HashMap::<String, Vec<String>>::new(),
+            local_modules: HashMap::<String, String>::new(),
+        }
+    }
+
+    fn make_test_client() -> TestClient {
+        let options = fake_options();
+        let output = fake_wasm_bindgen_output();
        let router = get_router(&options, output);
-        println!("{:?}", &router);
-        let client = TestClient::new(router);
+        TestClient::new(router)
+    }


I think for this test to be useful, we should probably make a fake WASM file and then compress that. At the moment we are providing a fake already compressed WASM file, so we aren't properly testing the code.

I gave this some thought and decided that for this particular case, a more targeted unit test was more appropriate, particularly since the primary goal is to make sure the router is able to choose the correct file. With regard to testing the compression, I was thinking that that could be addressed by another unit test in wasm_bindgen (although logically the compression could probably be extracted into a separate module).

Heavier-weight testing, I think, would be better placed into a separate PR, ideally with corresponding test automation.

This PR extracts the logic from `run_server` that creates a new Router into a helper function named `get_router` to make it easier to test without needing to start the full server. This also adds a simple test that verifies current behavior for returning a br compressed wasm.wasm file. This change is in preparation for adding support dynamically change the available compression.

heisencoder · 2022-11-24T18:11:44Z

I'm closing and moving this review over to #30 , since this change is the first commit in #30

Thanks @daxpedda for your thoughtful comments!

heisencoder commented Nov 23, 2022

View reviewed changes

heisencoder mentioned this pull request Nov 23, 2022

Feature/support gzip #30

Closed

daxpedda suggested changes Nov 23, 2022

View reviewed changes

heisencoder commented Nov 24, 2022

View reviewed changes

heisencoder added a commit to heisencoder/wasm-server-runner that referenced this pull request Nov 24, 2022

merged from jakobhellermann#29

a83967f

heisencoder requested a review from daxpedda November 24, 2022 01:36

daxpedda suggested changes Nov 24, 2022

View reviewed changes

heisencoder force-pushed the main branch from 8ec8ff9 to c7c5d84 Compare November 24, 2022 17:55

heisencoder closed this Nov 24, 2022

Extract get_router and add simple test. #29

Extract get_router and add simple test. #29

Uh oh!

Conversation

heisencoder commented Nov 23, 2022

Uh oh!

heisencoder commented Nov 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxpedda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heisencoder commented Nov 23, 2022

Uh oh!

heisencoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxpedda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heisencoder commented Nov 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants