Sketch: Top level mock wrapper

[tommy-gilligan]

A top-level mock wrapper seems like something that would be very useful.
#19

I've come up with something that works and have been using my own project to drive its implementation. https://github.com/tommy-gilligan/ab1024-ega/blob/main/src/test.rs (the tests here are a bit verbose but that is a different matter entirely)

^^ Emphasis on works because it is quite messy. This is just a sketch. I've taken a few shortcuts (deleted eh0, hard-coded SPI to u8 only, made some fields public).

Some goals I've aimed for here:

• keep existing API working and just have top level wrapper exist as extra functionality
• rely on existing infrastructure ie. Generic

IMHO the main ugliness here is that now there are multiple iterators involved for a single Mock. A Mock has its own iterator which it will use unless the Mock was created from a top level Hal Mock, in which case it uses that by reference. I guess this comes from keeping existing API working but trying to keep code duplication to a minimum but there are probably better ways of achieving this.

All in all, this is a direction towards having a top-level mock wrapper. I'm not sure it's the right direction but hope it helps in working out what that is. Happy to complete the implementation of a top-level mock wrapper but only once direction is decided. As such, love some feedback here.

Out of scope:

• check identity of resources (if an expectation is set on pin A, it still succeeds if it is pin B that gets used instead)

[dbrgn]

Thanks, it's an interesting experiment.

I think there are two aspects to a top-level wrapper:

1. There's a single API for everything.

While this may be nice, I don't think the benefit outweighs the complexity increase in the implementation. Right now every mock is independent, which makes things easier to maintain. Mocks can re-use the generic mock, but if an API is mocked where this doesn't make sense, it can be implemented differently. With a unified transaction API, we're kinda forced to make everything compatible with the top-level API (or vice versa).

2. Transactions for multiple subsystems can be asserted in one go

This one is cool! However, it does add more complexity to this project. I'm not yet sure whether this is a good tradeoff or not, although I like the API.

I think right now, as you said, the complexity comes from the fact that both APIs are retained. As long as there is a clear and easy migration path, I think breaking changes are still possible and legitimate. How would an API / implementation look like if we give up backwards compatibilty?

Maybe @newAM or @ryankurte have an opinion as well?

[Artur-Romaniuk]

1. There's a single API for everything.

While this may be nice, I don't think the benefit outweighs the complexity increase in the implementation. Right now every mock is independent, which makes things easier to maintain. Mocks can re-use the generic mock, but if an API is mocked where this doesn't make sense, it can be implemented differently. With a unified transaction API, we're kinda forced to make everything compatible with the top-level API (or vice versa).

I think that this could be a move in a generally good direction. Maybe this shouldn't come as a single PR, but I think if this is implemented carefully, overall benefits could be great.

We could start with a "top level" traits and make all impls compatible with it. This would also mean reviewing all existing code, possibly reducing code duplication between modules and standardizing some patterns. There are certainly big differences between modules, even though they usually operate similarly. This refactor could be a great first step.

An added benefit is that it could be easier for new developers to understand the code structure and not wonder why each module does the same thing differently (speaking from experience).

Also, an existence of a defined public trait would potentially add a way to provide user defined extensions, that seamlessly integrate with other mocks.

I believe that only when all modules are refactored, then a "top level" mock can be implemented, but that would unlock very powerful use cases for this library, e.g. expecting sequences of "transaction-delay-transaction" or "transaction-action-transaction".

[newAM]

Maybe @newAM or @ryankurte have an opinion as well?

It's definitely cool and I can see the usecase for it. The complexity of the added code will depend on how ordering is handled.

1. The top level needs to provide independent mutable structures that implement the HAL traits.
2. Something needs to check that the independent structs are expected in the correct order.

I don't yet have a good idea of how complex it will be in the end, parent-child relationships are always difficult in rust. There might be a path to simplifying the code by having a single type that implements all the HAL traits, or that might just be a hopeful wish 🤔

[asasine]

A top-level HAL mock would also mean PRs like #119 may be a relic, as a sufficient implementation could lend itself to implementing new HAL features across all traits simultaneously.

[dbrgn]

A top-level HAL mock would also mean PRs like #119 may be a relic, as a sufficient implementation could lend itself to implementing new HAL features across all traits simultaneously.

Not really. Even if you have a single wrapper type, you'd still need to impl all methods.

One way to simplify that would be a macro that implements every async method by calling the sync method. However, I doubt that it's worth the complexity. And it would also be applicable to a "every mock is independent" approach, not just to a unified API.