Skip to content

Fix issue where read_to_end() into provided rx_buf does not work with… #115

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

Merged
merged 2 commits into from
Mar 10, 2025

Conversation

rmja
Copy link
Member

@rmja rmja commented Feb 24, 2025

… tls

This is a fix for #114. The issue is that embedded-tls may provide a different buffer when calling buf_read() than we expect, so this PR allows us to force to not use its buffer.

// Also, if requests on embedded platforms fail with Error::Dns, then try to
// enable the "alloc" feature on embedded-tls to enable RSA ciphers.
let mut request = client
.request(Method::GET, "https://api.dictionaryapi.dev/api/v2/entries/en/orange")

Choose a reason for hiding this comment

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

Is there a way to test this without actually hitting this external API? We probably don't want the test to start failing if this website ever goes dark.

@rmja
Copy link
Member Author

rmja commented Feb 25, 2025

@lulf do you have any good idea on how we could setup a test harness that can recreate this issue?
The not-working behavior can be re-introduced by setting force_local_buffer: false. embedded-tls must be included in the test for the issue to show.

@rmja
Copy link
Member Author

rmja commented Feb 25, 2025

I wonder if there is something similar to Verify for rust, where we could load desired responses raw from disk, and then compare them to a desired and expected response.

@lulf
Copy link
Member

lulf commented Feb 25, 2025

Hmm, I'm not familiar with any appropriate mocking framework, but maybe one could use something like https://github.com/drogue-iot/embedded-tls/blob/main/tests/tlsserver.rs to reproduce it. It requires a bit more work to setup though.

@PhoebeSzmucer
Copy link

Are there more potential uses for something like https://github.com/drogue-iot/embedded-tls/blob/main/tests/tlsserver.rs in this repo, or is this going into the territory of testing embedded-tls itself?

@emil-e
Copy link

emil-e commented Mar 9, 2025

I also ran into this issue. Any updates on this?

@rmja
Copy link
Member Author

rmja commented Mar 9, 2025

Unless anyone has a good idea on how to proper test this, i guess we have to merge this as is. @lulf what do you think?

@PhoebeSzmucer
Copy link

Maybe we can just switch to calling a different API in the tests (something more stable, with less chance of being shut down)? Or delete the test entirely (as much as it pains me to say that).

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

Let's merge it, we don't run the ci / tests that often 🙈

@lulf lulf merged commit 23b3947 into drogue-iot:main Mar 10, 2025
1 check passed
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.

4 participants