rustc-generated test harness broken

[jrvanwhy]

In PR #103, the Termination implementation for () was removed, which results in the following errors for binaries compiled using cargo test:

error[E0277]: the trait bound `(): core::future::Future` is not satisfied
  |
  = note: required because of the requirements on the impl of `libtock::lang_items::Termination` for `()`

Unfortunately, this is not a trivial fix, as re-adding the Termination implementation for () gives:

error[E0119]: conflicting implementations of trait `lang_items::Termination` for type `()`:
  --> /usr/local/google/home/jrvanwhy/tock-on-titan/third_party/libtock-rs/src/lang_items.rs:49:1
   |
44 |   impl Termination for () {
   |   ----------------------- first implementation here
...
49 | / impl<T> Termination for T
50 | | where
51 | |     T: Future<Output = ()>,
52 | | {
...  |
56 | |     }
57 | | }
   | |_^ conflicting implementation for `()`
   |
   = note: upstream crates may add a new impl of trait `async_support::future::Future` for type `()` in future versions

[jrvanwhy]

If anyone has any ideas on how to fix this and allow main to return either () or a Future, please let me know.

One option that comes to mind is to drop support for a main() that returns a Future. This would be more consistent with upstream Rust, but presumably support for a Future-returning main() is desirable.

Another option is to create a FutureWrapper type that implements Termination, and allow an application to return the FutureWrapper type. It wouldn't support async fn main() directly, and I'm not sure what advantage that has over just calling block_on from main().

[torfmaster]

Does cargo test --lib do the trick for you? cargo test is broken for several reasons.

[torfmaster]

Regarding the async main: There one option I see (#104) to support both synchronuous and asynchronuous main functions in libtock-rs (it is from the design more or less the approach tokio uses). However, I see no direct benefit of actually supporting synchronuous main functions. My proposal would be: If for some reasons the lang_items cause trouble during testing of the library (which they should because we do no_std) we should replace them by mocked versions which do the right thing (as we do for the syscalls).

[Woyten]

cargo test adds the std crate including the #[lang = "termination"] item to the scope, s.t. the tests can be run. Unfortunately, for some reason, it also builds the main functions for the examples which are not designed to work with std. Is there any way to turn this off?

[torfmaster]

Two observations:
cargo test also fails prior to merging #103:

error: `#[panic_handler]` function required, but not found

error: language item required, but not found: `eh_personality`

error: `#[alloc_error_handler]` function required, but not found

I have no idea how to fix this.
If I delete all the examples I still get:

Doc-tests libtock
error: no global memory allocator found but one is required; link to std or add `#[global_allocator]` to a static item that implements the GlobalAlloc trait.

which could be fixed. My question is: what would cargo test do what cargo test --lib wouldn't?

[Woyten]

If you remove the dependency on the alloc crate (it's a leftover) and delete all examples, cargo test works.

[jrvanwhy]

Does cargo test --lib do the trick for you?

No, in both cases the rustc-generated main() returns ().

However, I see no direct benefit of actually supporting synchronuous main functions.

I see two:

1. Allowing use of Rust's unit test framework (which tock-on-titan currently uses).
2. Tock is used in workshops where new developers must be onboarded very quickly. Currently the workshops use libtock-c exclusively, but I think they'll eventually want to use libtock-rs as well. When that happens, minimizing the complexity of a hello world program will become very important.

There one option I see (#104) to support both synchronuous and asynchronuous main functions in libtock-rs (it is from the design more or less the approach tokio uses).

Looking again, I think that is a better approach. At the time I suggested implementing Termination for T: Future, I was missing two facts:

1. I didn't realize that would preclude implementing Termination for ()
2. I searched but missed that Tokio has support for an async main.

If for some reasons the lang_items cause trouble during testing of the library (which they should because we do no_std) we should replace them by mocked versions which do the right thing (as we do for the syscalls).

This sounds like a feature flag controlling what type Termination is. That seems workable to me. However, I prefer the original solution from #104 over that for consistency with Tokio and std.

cargo test adds the std crate including the #[lang = "termination"] item to the scope, s.t. the tests can be run.

This is only the case if you use upstream Rust's test crate. If you provide your own #![no_std] test crate then that won't occur.

[torfmaster]

Allowing use of Rust's unit test framework (which tock-on-titan currently uses).

What is your use case here exactly (maybe give a reference to an example repo)? We actually have unit tests in libtock-rs and they run in travis seamlessly. Are you sure, the async main function is the only obstruction against using Rust's unit test framework the way you want to? Following the discussions I expect more difficulties here.

Tock is used in workshops where new developers must be onboarded very quickly. Currently
the workshops use libtock-c exclusively, but I think they'll eventually want to use libtock-rs as
well. When that happens, minimizing the complexity of a hello world program will become very
important.

I think writing synchronuous code in an async main function is completely acceptable. The "async" can be completely ignored here.

This sounds like a feature flag controlling what type Termination is. That seems workable to me.

May be a #[cfg(test)] will already do the trick.

However, I prefer the original solution from #104 over that for consistency with Tokio and std.

I have to admit I'm a bit biased towards the async approach: I failed to write more complex applications (like controlling a water pump via ble and the temperature sensor) using the callback approach without cheating (i.e. creating two instances of the same GPIO pin). I think libtock-rs should offer a usable API - if it's synchronuous, I'm okay with it but currently I see no way how to make callbacks nice and see much more potential in the async approach.

[jrvanwhy]

What is your use case here exactly (maybe give a reference to an example repo)? We actually have unit tests in libtock-rs and they run in travis seamlessly.

The repo is google/tock-on-titan. We have 2 test apps (libraries compiled using cargo test): flash_test, nvcounter_test.

Are you sure, the async main function is the only obstruction against using Rust's unit test framework the way you want to? Following the discussions I expect more difficulties here.

Yes, I updated to the latest libtock-rs master, and the only change I needed to make to libtock-rs to keep our tests passing is to replace the Termination implementation with impl Termination for () { ... }. Our test crate is here: test_harness.

I think writing synchronuous code in an async main function is completely acceptable. The "async" can be completely ignored here.

This isn't a big deal, but it is additional cognitive overhead. In particular, developers new to Rust and/or Rust's async support may be tripped up by it. On its own, I would expect this to be okay, but the test breakage is a blocker for us.

This sounds like a feature flag controlling what type Termination is. That seems workable to me.

May be a #[cfg(test)] will already do the trick.

#[cfg(test)] will only work in the crate under test, not in its dependencies (i.e. libtock-rs will not see the test flag).

[torfmaster]

I just stumbled over #![reexport_test_harness_main] (in: https://os.phil-opp.com/testing/). It seems that the author faces a similar problem (having no main or having the wrong type of main).

[jrvanwhy]

I just stumbled over #![reexport_test_harness_main] (in: https://os.phil-opp.com/testing/). It seems that the author faces a similar problem (having no main or having the wrong type of main).

Unfortunately, that only reexports main rather than renames it, so I still get the compile error.

[jrvanwhy]

I've been trying to revive #104, but am running into some issues. Namely:

1. If main() does not parse, the error message does not point back to the source file. Presumably we want to support async main() because main() is nontrivial, otherwise main() could easily call core::executor::block_on() itself, but if main() is nontrivial then debugging parse failures without line number information will be difficult.
2. main() will be parsed by the version of syn that our procedural macro uses, not the compiler's version. This means that main() will be held to an older Rust version regardless of the toolchain version, so we'll need to update syn whenever the toolchain updates. This is somewhat nontrivial, especially as other Tock crates (e.g. elf2tab) depend on it.

I haven't looked into how Tokio's macro handles these problems. If it has a solution, then please point me to it. Otherwise, I think we should use another approach to support async main functions.

I'm out of time to work on this today. It may be possible to generate the wrapping main() function using a macro_rules! macro from libtock-rs; I'll look into that tomorrow.

[torfmaster]

It may be possible to generate the wrapping main() function using a macro_rules! macro from libtock-rs; I'll look into that tomorrow.

I tried this initially but I did choose to not use that approach because it is basically impossible to write unit tests for macro_rules macros.

[torfmaster]

Yes, I updated to the latest libtock-rs master, and the only change I needed to make to libtock-rs to keep our tests passing is to replace the Termination implementation with impl Termination for () { ... }. Our test crate is here: test_harness.

I had a look at these tests and I find that (from my perspective) this use case is rather non-canonical.

1. the tests treated as unit tests are more integration tests: They actually use a lot infrastructure (hardware, the whole library) but use the unit test framework.

2. These tests are impossible to run on in CI because the actual test runner should run in CI and only the test generating the output to be evaluated should be run on real hardware (or prefarably in a simulator). I wonder how these tests can be used for a test based development.

In my opinion this test runner worked by chance (and best great effort from your side, of course) but this is not a use case which I would necessarily support.

[torfmaster]

• If main() does not parse, the error message does not point back to the source file. Presumably we want to support async main() because main() is nontrivial, otherwise main() could easily call core::executor::block_on() itself, but if main() is nontrivial then debugging parse failures without line number information will be difficult.

This must be a solvable problem. I just didn't know of this problem and so didn't think it through. If it becomes consensus that async main is to be discarded I can invest some time here and fix this in #104.

• main() will be parsed by the version of syn that our procedural macro uses, not the compiler's version. This means that main() will be held to an older Rust version regardless of the toolchain version, so we'll need to update syn whenever the toolchain updates. This is somewhat nontrivial, especially as other Tock crates (e.g. elf2tab) depend on it.

I don't see the connection to elf2tab, actually. Afair it uses just stable rust. If syn matches the rust toolchain of tock which matches the toolchain of libtock-rs there should be no problem. I think a well-tested procedural macro should create appropriate code regardless of the version syn. Actually I see no reason why a non-matching syn version would cause any trouble if it is well-tested.

[jrvanwhy]

1. the tests treated as unit tests are more integration tests: They actually use a lot infrastructure (hardware, the whole library) but use the unit test framework.

There is a continuum between "unit test" and "system test"; Rust's test framework works perfectly fine for this.

2. These tests are impossible to run on in CI because the actual test runner should run in CI and only the test generating the output to be evaluated should be run on real hardware (or prefarably in a simulator). I wonder how these tests can be used for a test based development.

We currently run them by hand. Yes, this is hard to get into a CI. We intend to automate this eventually.

For local development, the need for physical hardware isn't a problem. Our development machines have the necessary hardware attached.

In my opinion this test runner worked by chance (and best great effort from your side, of course) but this is not a use case which I would necessarily support.

This use case is very valuable to us.

[jrvanwhy]

Sorry, mis-clicked a moment ago.

This must be a solvable problem. I just didn't know of this problem and so didn't think it through. If it becomes consensus that async main is to be discarded I can invest some time here and fix this in #104.

We need the Rust test framework to work; I have not found a way to fix it without requiring at least a minimal change to applications using async main.

[torfmaster]

Let just emphasize that I appreciate your effort for automatic testing - I personally have a passion for this subject.

We need the Rust test framework to work; I have not found a way to fix it without requiring at least a minimal change to applications using async main.

I'm thinking in terms of requirements here: Thinking of your code as of downstream to libtock-rs and tock I'm asking myself whether I want to have a test framework which runs on the target framework as a hard requirement to the library - I think the answer is yes. But I think using the rust test harness for the target architecture should not be a hard requirement - I'd prefer a solution like the PoC https://github.com/torfmaster/libtock-tests as a test runner for our integration tests in libtock-rs here. There the automation is already built-in (and could even be extended to run in qemu once the support for riscv is ready, for example). My reason for not wanting a running test harness on the target architecture is not that I necessarily want main to be async ( I would be okay with that) but rather that I want to want avoid building a library around downstream consumers test library requirements - they may have more implications for example future lang_items have to be formed in a certain way just to be compatible with the test framework.

This use case is very valuable to us.

I appreciate this a lot - I'd like to have real tests (running for each PR) for libtock-rs as well as they would have saved us from many regressions we had in the past.

There is a continuum between "unit test" and "system test"; Rust's test framework works perfectly fine for this.

I believe for system/integration tests the framework is right for the test runner but not for the test subject. Just imagine integration tests for a web server using a database. I think the same applies here.

In summary: I would like to build a framework for automatic testing - I currently see more potential in beeing automated and stable in the approach of having simple test binaries running on the target (i.e. qemu or real hardware) and a test runner doing the flashing, evaluation, and finally reporting to CI (Jenkings, travis).

[jrvanwhy]

I'd like to distinguish short-term changes versus long-term planning here. In the short term, we cannot update to libtock-rs's current master commit, which makes it difficult to contribute to libtock-rs. I am trying to bring us back up to master for further work.

Longer term, I am fine with switching to another test framework as long as doing so does not negatively impact our developer productivity.

I believe for system/integration tests the framework is right for the test runner but not for the test subject.

Please clarify what you mean by "test subject" here -- I don't understand your point.

[jrvanwhy]

Solved by #115