Skip to content

feat: initial std support #58

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 3 commits into from
Apr 25, 2025
Merged

feat: initial std support #58

merged 3 commits into from
Apr 25, 2025

Conversation

kaspar030
Copy link
Contributor

This PR allows to use embedded-test in non-no_std settings.

In Ariel OS, we're about to add a "native board", which is an Ariel variant that runs as regular unix process.
We'd like to use that to test our regular, no_std and embedded-test enabled examples and tests (wherever it makes sense).

Basically, this PR:

  1. factors out semihosting calls into a hosting module
  2. implements another hosting variant that uses std code
  3. adds examples/std for testing the std side

I've hacked together a libtest-mimic based runner: https://github.com/kaspar030/embedded-test-std-runner

I'm marking this as draft, feedback welcome.
I'd also be happy to move the std runner inside this directory, maybe it even helps for testing in CI.

@t-moe
Copy link
Contributor

t-moe commented Apr 1, 2025

Hi @kaspar030,
Thank you for your PR! It’s really cool to see embedded-test being used as an integral part of ariel-os.

However, I’m not sure the approach you’ve taken is the right direction. Since embedded-test aims to mimic on no_std what libtest does in a std environment, adding std support feels a bit counterintuitive.

Wouldn’t it be simpler to create a tests or test proc macro for ariel-os that delegates to embedded_test::tests or tokio::test / core::test, depending on whether std is available? I haven’t fully thought it through, but it might be worth considering.

Let me know your thoughts!

@kaspar030
Copy link
Contributor Author

However, I’m not sure the approach you’ve taken is the right direction. Since embedded-test aims to mimic on no_std what libtest does in a std environment, adding std support feels a bit counterintuitive.

Well, all libtest based tests have to be std. embedded-tests could also be called no-std-tests, a crate using it can be completely no_std, including its test cases.

IMO it is useful for e.g., library crates that want to support both std and no_std to have one set of tests that can be run on embedded hardware (in addition to std).
The current norm is to run the tests on std only due to libtest only supporting that, and maybe have another set of test applications that would be no_std.

Wouldn’t it be simpler to create a tests or test proc macro for ariel-os that delegates to embedded_test::tests or tokio::test / core::test, depending on whether std is available? I haven’t fully thought it through, but it might be worth considering.

Well, actually embedded-test just fits our no_std-on-linux needs quite perfectly, apart from the hard semihosting dependency. :) We'd also very much like to re-use our tests unmodified, and getting anything else to semantically match would probably not be worth the effort.

So unless I have already made the case properly, what would be your concerns still going with this?

I tried to keep the changes on the minimal side. In the semihosting case, code is mostly just moved into semihosting.rs, sometimes creating a new function.
There is some oddity around args (semihosting returning utf8 conv results, std flattens those). I could remove some std special cases there by adding the extra logic on the std side (in std.rd).

So I'd expect this to not cause any maintenance friction down the line, and I'd be sticking around to help in any case.

What this PR enables is adding more communication backends. Is unittesting on wasm a thing? :-)

Copy link
Contributor

@t-moe t-moe left a comment

Choose a reason for hiding this comment

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

We'd also very much like to re-use our tests unmodified, and getting anything else to semantically match would probably not be worth the effort.

Ok, I understand your point.

So unless I have already made the case properly, what would be your concerns still going with this?
....
So I'd expect this to not cause any maintenance friction down the line, and I'd be sticking around to help in any case.

Yeah, my main fear was maintenance friction down the line. But I agree that embedded-test could also be called no-std-test and I see the value of abstracting over the hosting interface. If you could remove the std special cases, so that we have a common interface... Then we can proceed with this.

@t-moe
Copy link
Contributor

t-moe commented Apr 4, 2025

thanks for the revision.
I'll only have time to look at it next week. But it plan to merge it.

@t-moe t-moe marked this pull request as ready for review April 25, 2025 19:56
@t-moe
Copy link
Contributor

t-moe commented Apr 25, 2025

@kaspar030
I think you need to kill the child process manually after invoking with_timeout here:
https://github.com/kaspar030/embedded-test-std-runner/blob/main/src/main.rs#L59C25-L59C61

At the moment running the tests on std keeps the it_timeouts testcase running at 100% CPU load....

See also: https://docs.rs/wait-timeout/0.2.1/wait_timeout/

(i'll merged this in the mean time and released as 0.6.1)

@t-moe t-moe merged commit 61f3250 into probe-rs:master Apr 25, 2025
10 of 11 checks passed
@kaspar030 kaspar030 deleted the std branch April 26, 2025 11:29
@bugadani
Copy link
Contributor

Just FYI 0.6.1 was a breaking change for default-features = false users.

@t-moe
Copy link
Contributor

t-moe commented Apr 28, 2025

Just FYI 0.6.1 was a breaking change for default-features = false users.

Ooops. I'm very sorry about that. I forgot that particular case. Do you recommend to revert, yank + release 0.7.0 ?

@bugadani
Copy link
Contributor

Normally yes, but we just have to merge the update PR and I'd just like to avoid any further noise.

@t-moe
Copy link
Contributor

t-moe commented Apr 28, 2025

Normally yes, but we just have to merge the update PR and I'd just like to avoid any further noise.

Alright. Then we leave it as it is. Sorry again.

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