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.

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

Begin replacing wit_bindgen with Rust SDK in integration tests #534

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

ejmg
Copy link
Contributor

@ejmg ejmg commented Jun 3, 2022

This PR addresses #216.

I'll need some feedback on the intended behavior for these tests to ensure the integrations are still checking for the desired behavior and functionality.

Copy link
Contributor

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ejmg ! This all looks great. I think we can open a new issue regarding the note you left to make the logic of these tests (in the component) a little more robust. Would you mind dropping the note you left into an issue linking this PR?

@ejmg
Copy link
Contributor Author

ejmg commented Jun 4, 2022 via email

@fibonacci1729
Copy link
Contributor

fibonacci1729 commented Jun 4, 2022

Sounds great! Most of the Fermyon folks are returning from an offsite this week so no rush! Happy camping! 🏕

@ejmg
Copy link
Contributor Author

ejmg commented Jun 5, 2022

Welp, camping plans changed. I'll open up a new issue with my comment and let me know if there is anything else I need to do on this PR before it gets merged.

@radu-matei
Copy link
Member

This looks great, thank you, @ejmg!
I left a couple of very minor comments, and the commit needs to be GPG signed and signed-off.
(https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits and https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---signoff)

@ejmg
Copy link
Contributor Author

ejmg commented Jun 6, 2022

Sorry for redundant commit, I missed the newline comment! I've signed off and verified my commits along with the suggested edits.

@ejmg ejmg changed the title (WIP) Replace wit_bindgen with Rust SDK in integration tests Begin replacing wit_bindgen with Rust SDK in integration tests Jun 6, 2022
@ejmg
Copy link
Contributor Author

ejmg commented Jun 7, 2022

@radu-matei and @fibonacci1729, I've updated this single test case to replicate the previous behavior of the test binary used by the integrations tests. To actually assert correctness, we would probably need to check the contents of the body but this is closer to correct behavior.

The one thing I'm wondering about right now (and as you'll see as a some NOTEs in the code) is that the return type of the component is no longer just a Response, but anyhow::Result. The error case for when the test asset file is not located is very different from the original code and I don't know how significant this is.

@radu-matei
Copy link
Member

Re: your note — you're not actually changing the response of the Wasm component itself.
Rather, the Rust SDK takes care of that, and checks the anyhow::Result<Response> and returns the right value.

(for a bit more context on the WIT design for the HTTP component, which we might want to change eventually — the Response is directly sent to the client — so if we ever have a Rust error, we automatically send a 500 status back, without the user manually doing that.)

In terms of the commits, would you please squash to a single commit that is signed?

Thanks, and apologies for the git friction here..

@ejmg ejmg force-pushed the update-tests-with-sdk-216 branch from 601419d to d59ffe5 Compare June 8, 2022 18:53
@ejmg
Copy link
Contributor Author

ejmg commented Jun 8, 2022

@radu-matei good to go?

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @ejmg!

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants