From 6ef55654f670d018f94b89bd711e37d81aac6fe2 Mon Sep 17 00:00:00 2001 From: Sam Carlberg Date: Wed, 17 Dec 2025 01:23:04 -0500 Subject: [PATCH] [cmd3] Fix commandv3 builders and add tests (#8482) Sequential group builder had an inverted if condition causing NPEs Parallel group builder was building parallel groups using an old ctor signature and creating groups that didn't match what was specified in the builder Test coverage has been added for both builder types --- .../org/wpilib/command3/ParallelGroup.java | 10 ++ .../wpilib/command3/ParallelGroupBuilder.java | 13 +-- .../org/wpilib/command3/SequentialGroup.java | 5 + .../command3/SequentialGroupBuilder.java | 2 +- .../command3/ParallelGroupBuilderTest.java | 83 ++++++++++++++ .../command3/SequentialGroupBuilderTest.java | 108 ++++++++++++++++++ 6 files changed, 212 insertions(+), 9 deletions(-) create mode 100644 commandsv3/src/test/java/org/wpilib/command3/ParallelGroupBuilderTest.java create mode 100644 commandsv3/src/test/java/org/wpilib/command3/SequentialGroupBuilderTest.java diff --git a/commandsv3/src/main/java/org/wpilib/command3/ParallelGroup.java b/commandsv3/src/main/java/org/wpilib/command3/ParallelGroup.java index bce639d2a3..a48429c41c 100644 --- a/commandsv3/src/main/java/org/wpilib/command3/ParallelGroup.java +++ b/commandsv3/src/main/java/org/wpilib/command3/ParallelGroup.java @@ -104,4 +104,14 @@ public final class ParallelGroup implements Command { public String toString() { return "ParallelGroup[name=" + m_name + "]"; } + + // package-private for testing + + Collection getRequiredCommands() { + return m_requiredCommands; + } + + Collection getOptionalCommands() { + return m_optionalCommands; + } } diff --git a/commandsv3/src/main/java/org/wpilib/command3/ParallelGroupBuilder.java b/commandsv3/src/main/java/org/wpilib/command3/ParallelGroupBuilder.java index f9ff315751..234723144c 100644 --- a/commandsv3/src/main/java/org/wpilib/command3/ParallelGroupBuilder.java +++ b/commandsv3/src/main/java/org/wpilib/command3/ParallelGroupBuilder.java @@ -21,7 +21,7 @@ import org.wpilib.annotation.NoDiscard; */ @NoDiscard public class ParallelGroupBuilder { - private final Set m_commands = new LinkedHashSet<>(); + private final Set m_optionalCommands = new LinkedHashSet<>(); private final Set m_requiredCommands = new LinkedHashSet<>(); private BooleanSupplier m_endCondition; @@ -44,7 +44,7 @@ public class ParallelGroupBuilder { requireNonNullParam(commands[i], "commands[" + i + "]", "ParallelGroupBuilder.optional"); } - m_commands.addAll(Arrays.asList(commands)); + m_optionalCommands.addAll(Arrays.asList(commands)); return this; } @@ -62,7 +62,6 @@ public class ParallelGroupBuilder { } m_requiredCommands.addAll(Arrays.asList(commands)); - m_commands.addAll(m_requiredCommands); return this; } @@ -103,7 +102,7 @@ public class ParallelGroupBuilder { public ParallelGroup named(String name) { requireNonNullParam(name, "name", "ParallelGroupBuilder.named"); - var group = new ParallelGroup(name, m_commands, m_requiredCommands); + var group = new ParallelGroup(name, m_requiredCommands, m_optionalCommands); if (m_endCondition == null) { // No custom end condition, return the group as is return group; @@ -125,16 +124,14 @@ public class ParallelGroupBuilder { String required = m_requiredCommands.stream().map(Command::name).collect(Collectors.joining(" & ", "(", ")")); - var optionalCommands = new LinkedHashSet<>(m_commands); - optionalCommands.removeAll(m_requiredCommands); // eg "(CommandA | CommandB | CommandC)" String optional = - optionalCommands.stream().map(Command::name).collect(Collectors.joining(" | ", "(", ")")); + m_optionalCommands.stream().map(Command::name).collect(Collectors.joining(" | ", "(", ")")); if (m_requiredCommands.isEmpty()) { // No required commands, pure race return named(optional); - } else if (optionalCommands.isEmpty()) { + } else if (m_optionalCommands.isEmpty()) { // Everything required return named(required); } else { diff --git a/commandsv3/src/main/java/org/wpilib/command3/SequentialGroup.java b/commandsv3/src/main/java/org/wpilib/command3/SequentialGroup.java index 7f05b524d5..28a54f00fd 100644 --- a/commandsv3/src/main/java/org/wpilib/command3/SequentialGroup.java +++ b/commandsv3/src/main/java/org/wpilib/command3/SequentialGroup.java @@ -77,4 +77,9 @@ public final class SequentialGroup implements Command { public String toString() { return "SequentialGroup[name=" + m_name + "]"; } + + // package-private for testing + List getCommands() { + return m_commands; + } } diff --git a/commandsv3/src/main/java/org/wpilib/command3/SequentialGroupBuilder.java b/commandsv3/src/main/java/org/wpilib/command3/SequentialGroupBuilder.java index 5a2dc0b6b0..f4234e90a1 100644 --- a/commandsv3/src/main/java/org/wpilib/command3/SequentialGroupBuilder.java +++ b/commandsv3/src/main/java/org/wpilib/command3/SequentialGroupBuilder.java @@ -83,7 +83,7 @@ public class SequentialGroupBuilder { */ public Command named(String name) { var seq = new SequentialGroup(name, m_steps); - if (m_endCondition != null) { + if (m_endCondition == null) { // No custom end condition, return the group as is return seq; } diff --git a/commandsv3/src/test/java/org/wpilib/command3/ParallelGroupBuilderTest.java b/commandsv3/src/test/java/org/wpilib/command3/ParallelGroupBuilderTest.java new file mode 100644 index 0000000000..8a32c3a4bc --- /dev/null +++ b/commandsv3/src/test/java/org/wpilib/command3/ParallelGroupBuilderTest.java @@ -0,0 +1,83 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.command3; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Set; +import org.junit.jupiter.api.Test; + +class ParallelGroupBuilderTest { + @Test + void optionalVarargNullArray() { + var builder = new ParallelGroupBuilder(); + assertThrows(NullPointerException.class, () -> builder.optional((Command[]) null)); + } + + @Test + void optionalNullInVarargs() { + var builder = new ParallelGroupBuilder(); + assertThrows(NullPointerException.class, () -> builder.optional((Command) null)); + } + + @Test + void optionalPopulatesOptionalOnly() { + var a = Command.noRequirements().executing(Coroutine::park).named("A"); + var b = Command.noRequirements().executing(Coroutine::park).named("B"); + + var group = new ParallelGroupBuilder().optional(a, b).withAutomaticName(); + + assertEquals(Set.of(a, b), group.getOptionalCommands()); + assertTrue(group.getRequiredCommands().isEmpty()); + assertEquals("(A | B)", group.name()); + } + + @Test + void requiringVarargNullArray() { + var builder = new ParallelGroupBuilder(); + assertThrows(NullPointerException.class, () -> builder.requiring((Command[]) null)); + } + + @Test + void requiringNullInVarargs() { + var builder = new ParallelGroupBuilder(); + assertThrows(NullPointerException.class, () -> builder.requiring((Command) null)); + } + + @Test + void requiringPopulatesRequiredOnly() { + var a = Command.noRequirements().executing(Coroutine::park).named("A"); + var b = Command.noRequirements().executing(Coroutine::park).named("B"); + + var group = new ParallelGroupBuilder().requiring(a, b).withAutomaticName(); + + assertEquals(Set.of(a, b), group.getRequiredCommands()); + assertTrue(group.getOptionalCommands().isEmpty()); + assertEquals("(A & B)", group.name()); + } + + @Test + void mixedRequiredAndOptional() { + var reqA = Command.noRequirements().executing(Coroutine::park).named("ReqA"); + var reqB = Command.noRequirements().executing(Coroutine::park).named("ReqB"); + var optX = Command.noRequirements().executing(Coroutine::park).named("OptX"); + var optY = Command.noRequirements().executing(Coroutine::park).named("OptY"); + + var group = + new ParallelGroupBuilder().requiring(reqA, reqB).optional(optX, optY).withAutomaticName(); + + assertEquals(Set.of(reqA, reqB), group.getRequiredCommands()); + assertEquals(Set.of(optX, optY), group.getOptionalCommands()); + assertEquals("[(ReqA & ReqB) * (OptX | OptY)]", group.name()); + } + + @Test + void namedWithNull() { + var builder = new ParallelGroupBuilder(); + assertThrows(NullPointerException.class, () -> builder.named(null)); + } +} diff --git a/commandsv3/src/test/java/org/wpilib/command3/SequentialGroupBuilderTest.java b/commandsv3/src/test/java/org/wpilib/command3/SequentialGroupBuilderTest.java new file mode 100644 index 0000000000..b97bfdac8b --- /dev/null +++ b/commandsv3/src/test/java/org/wpilib/command3/SequentialGroupBuilderTest.java @@ -0,0 +1,108 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.command3; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.List; +import org.junit.jupiter.api.Test; + +class SequentialGroupBuilderTest { + @Test + void andThenSingle() { + var c1 = Command.noRequirements().executing(Coroutine::park).named("C1"); + + var builder = new SequentialGroupBuilder(); + var sequence = builder.andThen(c1).named("Seq"); + assertInstanceOf(SequentialGroup.class, sequence); + assertEquals(List.of(c1), ((SequentialGroup) sequence).getCommands()); + assertEquals("Seq", sequence.name()); + } + + @Test + void andThenMultiple() { + var c1 = Command.noRequirements().executing(Coroutine::park).named("C1"); + var c2 = Command.noRequirements().executing(Coroutine::park).named("C2"); + + var builder = new SequentialGroupBuilder(); + var sequence = builder.andThen(c1, c2).named("Seq"); + + assertInstanceOf(SequentialGroup.class, sequence); + assertEquals(List.of(c1, c2), ((SequentialGroup) sequence).getCommands()); + assertEquals("Seq", sequence.name()); + } + + @Test + void andThenRepeated() { + var c1 = Command.noRequirements().executing(Coroutine::park).named("C1"); + var c2 = Command.noRequirements().executing(Coroutine::park).named("C2"); + + var builder = new SequentialGroupBuilder(); + var sequence = builder.andThen(c1).andThen(c2).named("Seq"); + + assertInstanceOf(SequentialGroup.class, sequence); + assertEquals(List.of(c1, c2), ((SequentialGroup) sequence).getCommands()); + assertEquals("Seq", sequence.name()); + } + + @Test + void andThenSingleNull() { + var builder = new SequentialGroupBuilder(); + assertThrows(NullPointerException.class, () -> builder.andThen((Command) null)); + } + + @Test + void andThenThenVarargNullArray() { + var builder = new SequentialGroupBuilder(); + assertThrows(NullPointerException.class, () -> builder.andThen((Command[]) null)); + } + + @Test + void andThenNullInVarargs() { + var builder = new SequentialGroupBuilder(); + assertThrows(NullPointerException.class, () -> builder.andThen((Command) null)); + } + + @Test + void untilReturnsParallelGroup() { + var c1 = Command.noRequirements().executing(Coroutine::park).named("C1"); + var builder = new SequentialGroupBuilder(); + var sequence = builder.andThen(c1).until(() -> false).named("Seq"); + assertInstanceOf(ParallelGroup.class, sequence); + assertEquals("Seq", sequence.name()); + } + + @Test + void namedWithNull() { + var builder = new SequentialGroupBuilder(); + assertThrows(NullPointerException.class, () -> builder.named(null)); + } + + @Test + void automaticNameWithEmptyGroup() { + var builder = new SequentialGroupBuilder(); + var sequence = builder.withAutomaticName(); + assertEquals("", sequence.name()); + } + + @Test + void automaticNameWithOneCommand() { + var c1 = Command.noRequirements().executing(Coroutine::park).named("C1"); + var builder = new SequentialGroupBuilder(); + var sequence = builder.andThen(c1).withAutomaticName(); + assertEquals("C1", sequence.name()); + } + + @Test + void automaticNameWithMultipleCommands() { + var c1 = Command.noRequirements().executing(Coroutine::park).named("C1"); + var c2 = Command.noRequirements().executing(Coroutine::park).named("C2"); + var builder = new SequentialGroupBuilder(); + var sequence = builder.andThen(c1, c2).withAutomaticName(); + assertEquals("C1 -> C2", sequence.name()); + } +}