[cmd] Add and use CommandScheduler.resetInstance()

[KangarooKoala]

Also makes the constructor private, since it was only used for the tests which now can use resetInstance(). Technically a breaking change for people putting their command unit tests in the commands package, but I doubt that many people would do that.

Closes #4866.

Python already has resetInstance(): https://github.com/robotpy/robotpy-commands-v2/blob/8480f6e3e3a5877e9489300ab8c5ec81703f2ad7/commands2/commandscheduler.py#L54.

Most of the changes are just removing try (CommandScheduler scheduler = new CommandScheduler()) {}, but there's a few other changes to the tests:

• SchedulerTest.registerSubsystem() and SchedulerTest.unregisterSubsystem() were both changed to use Subsystem instead of SubsystemBase since SubsystemBase would automatically register the subsystem with the singleton instance.
• A couple C++ lambdas that captured the scheduler by reference now capture this (which is by reference) since that's where the scheduler is stored now.
• CommandRequirementsTest.RequirementInterrupt and SchedulerTest.SchedulerInterruptLambdaCause had to be changed in C++ because the command deconstructors canceled the command at the end of their scope.

[github-actions]

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

[KangarooKoala]

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

Already done in Python (see link in PR comment).

[Starlight220]

Are there any tests that don't inherit from CommandTestBase etc?

[Starlight220]

I suggest it should null-op with a warning on a real robot. Is there a reason not to avoid the risk of a team accidentally resetting the scheduler in the middle of a match?

[KangarooKoala]

Sounds like a good idea- How could we detect we're running on a real robot versus in tests? I'd assume we wouldn't want to use isFMSAttached() because then it might lead to code working in the lab but not in an actual match, which would really suck.

I guess the way to really really avoid sneaky behavior changes is to always check (somehow) whether the call is from robot code (and not test code), but that's going to be really tricky for C++.

Another silly thought I had is to have a string parameter that must be "This method should only be called in unit tests. I promise I am not abusing this inside robot code." for the method to work, but that's not the most professional solution. (It would be pretty easy and funny, though.)

[Starlight220]

isReal()/isSimulation()?

[Starlight220]

Ditto

[Daniel1464]

I think the safest bet here might be a "scope function" of sorts. Such as:

CommandScheduler.withInstanceOverride(() -> {
    // do tests here
})

The advantage here i think is that when the scope is done executing, the default CommandScheduler instance can be reinstated, preventing sneaky errors from occuring. All of the test can occur within the scope block

[KangarooKoala]

Thanks for the suggestions, everyone! I've thought about a few different options, and I'm not completely sure which one to go for. Option C is mostly just for fun- While writing this out, I realized that it doesn't really have any advantages over the other options.

Option A: resetInstance()

• Java usage:

  CommandScheduler.resetInstance();
  testBody();

• C++ usage:

  CommandScheduler::ResetInstance();
  TestBody();

• Python usage:

  CommandScheduler.resetInstance()
  testBody()

• Pros:
  • Simple
  • Easy to be done automatically for tests
  • Already in Python
• Cons:
  • Affects code at a distance

Option B: withNewInstance() (credit @Daniel1464)

• Java usage:

  CommandScheduler.withNewInstance(() -> {
    testBody();
  });

• C++ usage:

  CommandScheduler::WithNewInstance([] {
    TestBody();
  });

• Python usage:

  @CommandScheduler.withNewInstance
  def _():
    testBody()

• Pros:
  • Effects are localized
  • Fits Kotlin idioms (This is just a fun observation and a very very low design priority)
• Cons:
  • Probably needs to be manually done in every unit test
  • Python usage is non-obvious (but can be documented)

Option C: Context manager

• Java usage:

  try (var _ = CommandScheduler.newInstanceScope()) {
    testBody();
  }

• C++ usage:

  auto newInstanceScope = CommandScheduler::NewInstanceScope();
  TestBody();

• Python usage:

  with CommandScheduler.newInstanceScope():
    testBody()

• Pros:
  • Effects are localized
• Cons:
  • Probably needs to be manually done in every unit test
  • Uses more advanced language features (but those can be taught / we can include examples in documentation)
  • More complex to implement (Requires defining a helper type)

[Starlight220]

Either way I think too much stuff reaches into the scheduler statically to have swapping out the scheduler be clean.

As for C, a separate type isn't needed -- CommandScheduler already implements AutoCloseable (and parallels); it's just that implementation is focused on the Sendable implementation. Having the close() method reset the state would be quite clean. Whether it swaps the instance or just resets the same one is up for discussion (I'm leaning towards the former)

[Daniel1464]

• Fits Kotlin idioms (This is just a fun observation and a very very low design priority)

Lol i actually wasnt designing it for this purpose, just thought that it would be a good idea to have some cleanup available at the end. Honestly though I prefer the try with resources method now that u bring it up.

[KangarooKoala]

Ended up going with resetInstance() because it matches what Python already has and is the simplest to implement. If there's a strong consensus that it should be something else I'll change it, but right now there doesn't seem to be strong opinions to do something else.

As for C, a separate type isn't needed

Yes, it would- I didn't explain the idea behind C much, but the idea was to have a separate context-manager/try-with-resources type that would on creation swap out the command scheduler implementation and on destruction swap it back. I didn't want to put the scope management in the CommandScheduler class because then we would need to let users make a new instance in some way to use the scope management, which could cause issues because then people could use a command scheduler instance that isn't the same one used by ProxyCommand and co.