Add Json Format, Intellij Options and make multi-threaded optional #34
Conversation
|
@LukasKalbertodt |
|
Could you take a moment and review this PR in the coming weeks? This step is crucial for progressing with our other PRs, as this is the only dependency not yet upstreamed to crates.io. Your feedback would be greatly appreciated and will help keep our project moving forward smoothly. Thank you for your time and assistance! |
|
Hi @t-moe, thanks for the PR! I don't have a lot of time to work on libtest-mimic currently, so my reviews won't be very quick. I just cherry picked the JSON and no-op args parts of this PR and merged them. They are also already released as 0.7.0! So thank you for that. What remains in this PR is the "remove send bound" and "add trial lifetime param". Before I ask you to rebase, could you motivate these changes for me? The first one part adds quite a bit of complexity, and the second one even complicates the API. So I'm naturally hesitant to merge that, as I cannot think of a use case for this. So yeah, please explain :) |
There was a problem hiding this comment.
Just two random things I noticed already (just in case you convince me that I want to merge your changes). Also: please remove all rustfmt changes from this PR when you rebase it. See the comment here for more information.
src/lib.rs
Outdated
| #[cfg(feature = "multithreaded")] runner: Box<dyn FnOnce(bool) -> Outcome + Send>, | ||
| #[cfg(not(feature = "multithreaded"))] runner: Box<dyn FnOnce(bool) -> Outcome>, |
There was a problem hiding this comment.
Can you just define a type alias somewhere (with cfg attributes) so that the cfg attributes only have to be in one place?
Cargo.toml
Outdated
| [dependencies] | ||
| clap = { version = "4.0.8", features = ["derive"] } | ||
| threadpool = "1.8.1" | ||
| threadpool = { version = "1.8.1", optional = true} |
|
Thank you @LukasKalbertodt for taking the time to cherry-pick and release parts of this PR!
We use libtest-mimic to enable integration-testing of embedded devices. More specifically, libtest-mimic is used in probe-rs which in turn controls targets running embedded-test. probe-rs first flashes a binary containing all tests onto the target, and then for each test resets the device and then runs the test (communicating via semihosting). Our primary need was to disable the Because we setup the connection with the Debug Adapter and the target only once, and not before each test, we need to pass some references into the test function. Removing the send bounds and allowing to specify the lifetime, allowed this without hacks on our side, hence this change. I think from the perspective of a seasoned rust developers these modifications would enhance the library's versatility, making it more adaptable for various use cases. However, I recognize that from a teaching standpoint, the balance between increased flexibility and the resultant complexity might not seem as favorable. I greatly appreciate your time in reviewing these changes and am more than willing to engage in further discussions or revisions to align this proposal with the library's vision and user base. |
|
Sorry for my delay getting back to this. Unfortunately, at this point I will have to reject your remaining (multi-threaded related) changes. As we already said: they do complicate the API and implementation, which is not ideal. I understand the use case and see that there is a need for it, but I suspect that it's rather niche. And simply given the fact that I have not gotten around to finding a good way to incorporate your changes by now, I doubt it will happen anytime soon, if ever. So yeah, again, sorry for rejecting this part, and thanks a lot for your other changes that are already merged and released! |
This PR:
Closes #1
Closes #25
Makes #33 obsolete