[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

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<Command> getRequiredCommands() {
+    return m_requiredCommands;
+  }
+
+  Collection<Command> getOptionalCommands() {
+    return m_optionalCommands;
+  }
 }

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<Command> m_commands = new LinkedHashSet<>();
+  private final Set<Command> m_optionalCommands = new LinkedHashSet<>();
   private final Set<Command> 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 {

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<Command> getCommands() {
+    return m_commands;
+  }
 }

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;
     }

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

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