Currently, tests use command class constructors to make commands (e.g., new RunCommand(() -> {})). This deviates from the recommended coding style and is not as readable. The tests should be updated to use command factories and decorators instead where applicable. The list below has every spot I noticed that should be changed (excluding spots where the class itself is being tested, such as the class-specific tests (e.g., ConditionalCommandTest) and RobotDisabledCommandTest).
Spots to change (in Java, haven't checked C++ yet):
CommandDecoratorTest: (4x) new RunCommand(() -> {}) -> Commands.idle()
CommandDecoratorTest: (7x) new InstantCommand() -> Commands.none()
CommandDecoratorTest: new InstantCommand(() -> condition.set(true)) -> Commands.runOnce(() -> condition.set(true))
CommandDecoratorTest: (2x) new WaitUntilCommand(finish::get) -> Commands.waitUntil(finish::get)
CommandDecoratorTest: (2x) new WaitUntilCommand(() -> false) -> Commands.idle()
CommandDecoratorTest: (2x) new InstantCommand(() -> hasRun.set(true)) -> Commands.runOnce(() -> hasRun.set(true))
CommandRequirementsTest: new RunCommand(() -> {}, requirement) -> Commands.idle(requirement) or requirement.run(() -> {})
CommandRequirementsTest: new WaitUntilCommand(() -> false) -> Commands.idle()
ConditionalCommandTest: new InstantCommand(() -> {}, system3) -> Commands.runOnce(() -> {}, system3) or system3.runOnce(() -> {})
ConditionalCommandTest: (16x) new WaitUntilCommand(() -> false) -> Commands.idle()
MultiCompositionTestBase: (24x) new WaitUntilCommand(() -> false) -> Commands.idle()
MultiCompositionTestBase: (2x) new InstantComand(() -> {}) -> Commands.none()
ParallelDeadlineGroupTest: (3x) new InstantCommand(() -> {}) -> Commands.none()
ParallelRaceGroupTest: new SequentialCommandGroup(command1, command2) -> Commands.sequence(command1, command2) or command1.andThen(command2)
ProxyCommandTest: new WaitUntilCommand(cond::get) -> Commands.waitUntil(cond::get)
ScheduleCommandTest: new SequentialCommandGroup(new InstantCommand(), scheduleCommand) -> Commands.sequence(Commands.none(), scheduleCommand) or Commands.none().andThen(scheduleCommand) or scheduleCommand.beforeStarting(Commands.none())
ScheduleCommandTest: new RunCommand(() -> {}) -> Commands.idle()
SchedulerTest: new InstantCommand() -> Commands.none()
SchedulerTest: (3x) new WaitCommand(10) -> Commands.idle() (We don't actually care about the command ending after 10 seconds)
SchedulingRecursionTest: (3x) new RunCommand(() -> hasOtherRun.set(true), requirement) -> Commands.run(() -> hasOtherRun.set(true), requirement) or requirement.run(() -> hasOtherRun.set(true))
SchedulingRecursionTest: new RunCommand(() -> {}) -> Commands.idle()
SchedulingRecursionTest: (6x) new InstantCommand(() -> {}, requirement) -> Commands.runOnce(() -> {}, requirement) or requirement.runOnce(() -> {})
SelectCommandTest: new InstantCommand(() -> {}, system3) -> Commands.runOnce(() -> {}, system3) or system3.runOnce(() -> {})
SingleCompositionTestBase: (2x) new WaitUntilCommand(() -> false) -> Commands.idle()
SingleCompositionTestBase: (3x) new InstantCommand() -> Commands.none()
SingleCompositionTestBase: new RunCommand(() -> {}) -> Commands.idle()
Other notes:
There's a decent number of FunctionalCommand instances that can't be directly replaced with command factories. (We could reproduce the logic with some compositions, but that increases the logic complexity of the test)
Commands.none() can't accept any requirements, which may be a separate issue. (It also might not- I could also see the case for it not being able to accept any requirements, such as the meaning of a "do nothing command")
A Subsystem.idle() would make sense, but belongs in a separate issue.
Currently, tests use command class constructors to make commands (e.g.,
new RunCommand(() -> {})). This deviates from the recommended coding style and is not as readable. The tests should be updated to use command factories and decorators instead where applicable. The list below has every spot I noticed that should be changed (excluding spots where the class itself is being tested, such as the class-specific tests (e.g.,ConditionalCommandTest) andRobotDisabledCommandTest).Spots to change (in Java, haven't checked C++ yet):
new RunCommand(() -> {})->Commands.idle()new InstantCommand()->Commands.none()new InstantCommand(() -> condition.set(true))->Commands.runOnce(() -> condition.set(true))new WaitUntilCommand(finish::get)->Commands.waitUntil(finish::get)new WaitUntilCommand(() -> false)->Commands.idle()new InstantCommand(() -> hasRun.set(true))->Commands.runOnce(() -> hasRun.set(true))new RunCommand(() -> {}, requirement)->Commands.idle(requirement)orrequirement.run(() -> {})new WaitUntilCommand(() -> false)->Commands.idle()new InstantCommand(() -> {}, system3)->Commands.runOnce(() -> {}, system3)orsystem3.runOnce(() -> {})new WaitUntilCommand(() -> false)->Commands.idle()new WaitUntilCommand(() -> false)->Commands.idle()new InstantComand(() -> {})->Commands.none()new InstantCommand(() -> {})->Commands.none()new SequentialCommandGroup(command1, command2)->Commands.sequence(command1, command2)orcommand1.andThen(command2)new WaitUntilCommand(cond::get)->Commands.waitUntil(cond::get)new SequentialCommandGroup(new InstantCommand(), scheduleCommand)->Commands.sequence(Commands.none(), scheduleCommand)orCommands.none().andThen(scheduleCommand)orscheduleCommand.beforeStarting(Commands.none())new RunCommand(() -> {})->Commands.idle()new InstantCommand()->Commands.none()new WaitCommand(10)->Commands.idle()(We don't actually care about the command ending after 10 seconds)new RunCommand(() -> hasOtherRun.set(true), requirement)->Commands.run(() -> hasOtherRun.set(true), requirement)orrequirement.run(() -> hasOtherRun.set(true))new RunCommand(() -> {})->Commands.idle()new InstantCommand(() -> {}, requirement)->Commands.runOnce(() -> {}, requirement)orrequirement.runOnce(() -> {})new InstantCommand(() -> {}, system3)->Commands.runOnce(() -> {}, system3)orsystem3.runOnce(() -> {})new WaitUntilCommand(() -> false)->Commands.idle()new InstantCommand()->Commands.none()new RunCommand(() -> {})->Commands.idle()Other notes:
FunctionalCommandinstances that can't be directly replaced with command factories. (We could reproduce the logic with some compositions, but that increases the logic complexity of the test)Commands.none()can't accept any requirements, which may be a separate issue. (It also might not- I could also see the case for it not being able to accept any requirements, such as the meaning of a "do nothing command")Subsystem.idle()would make sense, but belongs in a separate issue.