[cmd] Add and use CommandScheduler.resetInstance() #7584
Conversation
|
This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR. |
github-actions
bot
added
the
component: command-based
Already done in Python (see link in PR comment). |
| * Resets the Scheduler instance, which is useful for testing purposes. This should not be called | ||
| * from user code. | ||
| */ | ||
| public static synchronized void resetInstance() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
| @@ -93,6 +93,10 @@ CommandScheduler& CommandScheduler::GetInstance() { | |||
| return scheduler; | |||
| } | |||
|
|
|||
| void CommandScheduler::ResetInstance() { | |||
| std::make_unique<Impl>().swap(GetInstance().m_impl); | |||
|
I think the safest bet here might be a "scope function" of sorts. Such as: 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 |
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()andSchedulerTest.unregisterSubsystem()were both changed to useSubsysteminstead ofSubsystemBasesinceSubsystemBasewould automatically register the subsystem with the singleton instance.this(which is by reference) since that's where the scheduler is stored now.CommandRequirementsTest.RequirementInterruptandSchedulerTest.SchedulerInterruptLambdaCausehad to be changed in C++ because the command deconstructors canceled the command at the end of their scope.