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()); + } +}