Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: add test cases for serve #15

Merged
merged 2 commits into from
Jul 25, 2022
Merged

test: add test cases for serve #15

merged 2 commits into from
Jul 25, 2022

Conversation

FrankYang0529
Copy link
Contributor

@FrankYang0529 FrankYang0529 commented Jul 20, 2022 •
edited
Loading

resolves #1 #2

lann
lann reviewed Jul 20, 2022
View reviewed changes
src/lib.rs Outdated
@@ -66,6 +66,7 @@ fn serve(req: Request) -> Result<Response> {
.get(PATH_INFO_HEADER)
.expect("PATH_INFO header must be set by the Spin runtime")
.to_str()?;
println!("path: {}", path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray println

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it. Thank you.

@FrankYang0529
Copy link
Contributor Author

Hi reviewers, this is my first time writing test cases for a wasm target. Thanks for your review and guidance. I have two questions.

  1. Not sure whether it's right to use wasm-pack test --node. Is there a convenient way to use the cargo test for testing wasm?
  2. I would like to write a positive test case for the serve function, but I can't get a 200 response even though I use the complete path.

@lann
Copy link
Contributor

lann commented Jul 20, 2022

Thanks for the PR!

It doesn't look like these tests really depend on being run in a Wasm environment; I tested locally by replacing #[wasm_bindgen_test] with #[test] and running cargo test --target=x86_64-unknown-linux-gnu which worked fine. We should see if there is a nice way to set up .cargo/config.toml so that tests don't need to cross-compile.

@lann
Copy link
Contributor

lann commented Jul 20, 2022

I don't see anything obvious in the cargo docs to simplify it, but for the purposes of this PR I think we can use this slightly awkward command in the Makefile: cargo test --target=$$(rustc -vV | sed -n 's|host: ||p')

@FrankYang0529
Copy link
Contributor Author

I don't see anything obvious in the cargo docs to simplify it, but for the purposes of this PR I think we can use this slightly awkward command in the Makefile: cargo test --target=$$(rustc -vV | sed -n 's|host: ||p')

Yeah, this is a good idea. I just saw that cargo merged multiple targets a few days ago. Probably, we can also try it?

rust-lang/cargo#8176

@FrankYang0529
Copy link
Contributor Author

Hi @lann, I update test cases and add CI. Please help me review when you have time. Thank you.

lann
lann reviewed Jul 22, 2022
View reviewed changes
.github/workflows/build.yml Outdated
@@ -0,0 +1,52 @@
# yaml-language-server: $schema=https://json.schemastore.org/github-workflow.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified a lot for this repo - much of what we want to test for spin doesn't apply here. If you'd like to drop this from your PR I can add something simple in a followup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive me if I'm misreading, but it looks like you dropped this line and kept the rest of the file. If we keep the file (which is what I think @lann was questioning), then we should keep this line. The reason is that this declaration avoids squigglies when editing the workflow in VS Code if you have the YAML extension installed. The YAML language server seems to recognise certain filenames and infer a schema, and for build.yaml it infers something other than GH workflow and complains about the 'wrong' entries.

Again, if I've misunderstood then please ignore me!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized my comment was unclear so I refactored in #17. I just opened #18 to add the comment back

@FrankYang0529
ci: init
28b3c26 
Signed-off-by: Frank Yang <[email protected]>
@lann
Copy link
Contributor

lann commented Jul 25, 2022

Thanks for the PR!

@lann lann merged commit e0f37e0 into fermyon:main Jul 25, 2022
@FrankYang0529 FrankYang0529 deleted the add-test branch July 25, 2022 13:09
@FrankYang0529 FrankYang0529 mentioned this pull request Aug 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lann lann lann approved these changes

@itowlson itowlson itowlson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add tests
3 participants
@FrankYang0529 @lann @itowlson