Skip to content
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

Subclass Vendor Objects Rather than Mocking Them #101

Open
CoolSpy3 opened this issue May 28, 2024 · 1 comment
Open

Subclass Vendor Objects Rather than Mocking Them #101

CoolSpy3 opened this issue May 28, 2024 · 1 comment

Comments

@CoolSpy3
Copy link
Member

This issue is long and contains a bunch of different ideas. It might be better to work on it as multiple separate issues, but for now, we can just use it to start discussion.

We arrived at the current mocking framework for our simulation classes because, in the past, initializing vendor devices in a sim environment led to crashes. However, as discussed in #44, this is no longer the case. Thus, someone should look into whether it would be beneficial to move to a framework where we instead subclass the relevant vendor objects.

Benefits

  • It would better practice. Mockito is designed for testing; not production. Thus, we would probably see significantly less overhead, among other things.
  • It would make it more clear how each of the mock classes relate to each of the vendor classes, and how the calling and non-implemented method semantics work.
  • It could prevent undefined behavior in the case that a non-mocked vendor method is called.
  • We would be much more tightly coupled to the vendor libraries. At the moment, if a method name/argument changes, the method calls fall-through silently. In this new framework, the build would fail after one of these changes, forcing us to update the method.

Costs

  • We gain less control over how each class is simulated. We have to accept the existence of whatever simulation backend/inheritance hierarchy is created by the vendors.
  • The mock classes have to extend a specific vendor class. This means that we can no longer have classes like MockedMotorBase and MockPhoenixController that implement a bunch of methods common to multiple controllers. (Side note: MockPhoenixController should probably be updated to delegate some of its logic to MockedMotorBase.) Instead, the clearest option would probably to have these classes create a MockedMotorBase (or other class) internally, and proxy the method calls, adding a bunch of extra boilerplate (if we go this route, we should probably rename the base classes)
@Override
public Object method() {
    return motorBaseSim.method();
}

(We might be able to do something like write an annotation processor that essentially does what we use Mockito for now, but at compile time and with our own classes, but that would add back some of the complication that we're trying to move away from with this issue.)

  • Phoenix creates their own SimDevice objects which could conflict with ours (or at least clutter up the environment).

Implementation note: Phoenix already provides SimCollection objects for many of their devices. For these devices, it might be preferable to instead adopt a model similar to the current implementation of MockedCANCoder. Depending on how much CTRE has implemented (which someone should check), it might prevent the need to handle a bunch of features ourselves.

Furthermore, while Phoenix devices don't exactly conform to the WPILib spec, they're pretty close. If all of their API endpoints are fully situatable (which someone should check), It might be worth trying to contact them to ask if they would bring it up to spec. This would remove the need for us to do any work whatsoever for CTRE devices. In my (very quick) skim, the only things I noticed were

  1. CANcoders have their position set as an output rather than an input. Imo, they should rewrite this to be an input and set it using a SimDevice rather than directly, but given how their API works, I see why they'd do this.
  2. The values introduced in Add CAN message schema to wpilib-ws.yaml and add 2 CANMotor props wpilibsuite/allwpilib#6651 are not implemented. (Although those were added <1 wk ago, so I'd be surprised if they were.)
@brettle
Copy link
Member

brettle commented May 28, 2024

Another potential disadvantage to the subclassing approach is that if any of the classes we want to subclass are final, or any of the methods we need to override are final, the subclassing approach won't work. Moreover, even if that isn't a problem at the time we decide to pursue subclassing, if the vendor later makes such things final, we would likely need to revert to using Mockito.

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

No branches or pull requests

2 participants