libtest-mimic icon indicating copy to clipboard operation
libtest-mimic copied to clipboard

Add Json Format, Intellij Options and make multi-threaded optional

Open t-moe opened this issue 2 years ago • 4 comments

This PR:

  • Aims to complete #33 + #28, fixing the pending issues
  • Gate multi-threaded execution behind a feature, to drop the Send bound if it is not needed
  • Accept a lifetime parameter to allow non-static Test/Bench functions

Closes #1 Closes #25 Makes #33 obsolete

t-moe avatar Dec 22 '23 10:12 t-moe

@LukasKalbertodt I'm not sure what you meant in https://github.com/LukasKalbertodt/libtest-mimic/pull/33/files#r1235742883 The remaining issues of that PR I've tried to address.

t-moe avatar Dec 22 '23 10:12 t-moe

Hi @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!

t-moe avatar Jan 09 '24 10:01 t-moe

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 :)

LukasKalbertodt avatar Jan 14 '24 16:01 LukasKalbertodt

Thank you @LukasKalbertodt for taking the time to cherry-pick and release parts of this PR!

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 :)

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 --test-threads command line option, as concurrent tests aren't feasible on embedded devices.

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.

t-moe avatar Jan 15 '24 08:01 t-moe