Post-season cleanup: 11 bug fixes + 66 tests + CI#2
Open
sjqtentacles wants to merge 12 commits into
Open
Conversation
…el forks) Adds test deps and a base class for HAL-requiring tests. Layer A (pure-logic) tests do not extend WpilibTestBase so the inner loop stays sub-second; Layer C tests extend it for HAL/sim access. Also chmod +x gradlew (was non-executable in the repo). Co-authored-by: Cursor <cursoragent@cursor.com>
The red climb tower was declared as Pose2d(15.478, 5201, 0deg) — y is 1000x what it should be (5.201 m). Any auto using alignToClimb() on the red side would have tried to drive ~5 km off the field. FieldBoundsTest reflectively walks every public static Translation2d/Pose2d on Field and asserts the coordinates lie within [0, LENGTH] x [0, WIDTH]. It catches the typo and any future coordinate typos. Co-authored-by: Cursor <cursoragent@cursor.com>
…ls; delete dupes
The chooser had three addOption() calls whose labels said "...Bravo..."
but whose PathPlannerAuto loaded the "...Alpha..." auto file. Delete
all three — AutoBuilder.buildAutoChooser() already auto-loads every
.auto file on disk (including Double Swipe Bravo Auto.auto) by its
real name, so the chooser is now honest by default.
AutoChooserConsistencyTest parses Auto.java source for two patterns:
1. every new PathPlannerAuto("X", ...) → X.auto exists on disk
2. every addOption("Label", new PathPlannerAuto("Name", ...)) → Label == Name
Catches future mislabel regressions.
Co-authored-by: Cursor <cursoragent@cursor.com>
Extracts the named-command list out of Robot.Robot()'s body into a new
NamedCommandRegistry class. Robot.Robot() now just calls
Auto.initialize(NamedCommandRegistry.all()).
Two new tests:
1. every_named_command_referenced_in_an_auto_is_registered: walks
every .auto JSON, collects {"type":"named","data":{"name":"X"}}
references, asserts each X appears in NamedCommandRegistry.NAMES.
Would have caught any auto referencing an unregistered command
(which currently no-ops silently in a match).
2. every_path_referenced_in_an_auto_exists_on_disk: same idea for
{"type":"path","data":{"pathName":"X"}} -> X.path on disk.
Co-authored-by: Cursor <cursoragent@cursor.com>
Moves the AprilTag accept/reject + stddev computation into a pure-logic
util class so it can be exhaustively unit-tested without HAL or mocks.
acceptEstimate() now reads as 7 lines: evaluate -> setStdDevs -> publish.
Also cleans up:
- Three stale SmartDashboard.putBoolean("Accepted", false) writes
that lived between if-statements and were either dead or always
overwritten by the final write.
- Bug fix: omega threshold was `> 1.5` (sign-sensitive); VisionFilter
uses Math.abs(omega) > 1.5, so a fast negative rotation is now
also rejected.
VisionFilterTest has 10 tests covering each boundary and the stddev
quadratic. ~50ms wall-clock.
Co-authored-by: Cursor <cursoragent@cursor.com>
…esis Replaces the sticky-once-true isAtspeed boolean in ShootCommand with a per-loop hysteresis latch: - not firing -> fire iff |measured - target| < enterTol (0.95) - firing -> stop iff |measured - target| >= exitTol (1.5) The latch lives in a new pure-logic util ShooterControlPolicy.compute(), making the decision exhaustively testable with no HAL or mocks. ShootCommand just plumbs measured velocity in and the Decision out. Note: this fix only matters once the velocity sensor reports a meaningful value — currently (until cycle 6) Shooter.getRollerVelocity() averages two opposed motors and returns ~0, so the threshold rarely trips. Cycle 6 fixes the sensor side. 12-test ShooterControlPolicyTest covers: table interpolation (incl. clamping above/below table bounds), each hysteresis transition, and the asymmetric band width guarantee. Co-authored-by: Cursor <cursoragent@cursor.com>
…ith opposed follower) The previous cachedRollerVelocity = (vel_roller + vel_roller_2)/2 line silently cancelled to ~0 at speed because m_Shooter_2 is set up as a Follower with MotorAlignmentValue.Opposed, so its signed rotor velocity is the negation of the leader's. This is the root cause of ShootCommand never tripping its at-speed threshold: |0 - target| > enterTol always. Extracted the aggregation into Shooter.aggregateRollerVelocity(leader, follower) so it can be unit-tested with no HAL and no Phoenix6 sim, which proved unreliable in headless JUnit (status-signal pipeline doesn't propagate setRotorVelocity injections cleanly). Layer A test covers: nominal opposed pair, at rest, ignoring follower value, positive-leader symmetry, sign preservation. Note: rejected an earlier Phoenix6-sim integration test attempt (Layer C) because Phoenix6 sim status-signal updates require either a running CAN device manager or specific update-frequency setup we can't reliably get inside a headless test JVM. The pure-logic extraction gives equivalent regression coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
Adds Hopper.stop() — a synchronous void that calls m_Hopper.stopMotor()
directly — for callers that need the motor to actually stop right now
(inside other commands' execute()/end() methods). The existing
Hopper.Stop() (capital S, returns a Command) is preserved but is now
intended only for setDefaultCommand(...) in Binds.
Switches the five in-code callers in Shooter.idle/ShootPass/Stop and
ShootCommand.end from Hopper.get().Stop() (whose returned Command was
discarded — so the motor never actually stopped) to Hopper.get().stop().
HopperStopContractTest pins this contract:
- reflection: Hopper.stop() exists as a public void method.
- source-scan: no .java file (outside Binds.java + Hopper.java
itself) is allowed to call Hopper.get().Stop() — only stop() is.
Co-authored-by: Cursor <cursoragent@cursor.com>
Swerve was caching the climb-tower target pose at construction time:
Pose2d targetPose = Field.Alliance_Find.climb_tower;
But Swerve is built before the FMS reports the alliance, so this
captured the default (blue) pose forever. alignToClimb() then passed
the stale pose to AutoBuilder.pathfindToPose, so on the red alliance
the climb-align command would drive to the blue tower.
Fix: delete the cached field; wrap alignToClimb() in Commands.defer so
the climb_tower lookup happens at schedule-time (alliance known by
then). Requirements still declared via Set.of(this).
AlignToClimbTest pins both:
- reflection: no non-static Pose2d field on Swerve names "target",
"climb", or "tower" (anyone who re-adds a cache fails this test).
- source-scan: alignToClimb's body must contain "Commands.defer(".
Co-authored-by: Cursor <cursoragent@cursor.com>
- Robot.java: add explanatory comment for 6.0V brownout threshold (deliberately below WPILib default of 6.75 V for sag headroom). - Robot.java: remove redundant Intake.SlotZeroConfigIntake() call at init — Intake() constructor already applies the full cfg_Pivot config; the partial re-apply was at best a no-op and at worst a clobber of any controller-side overrides. - Intake.SlotZeroConfigIntake(): mark @deprecated with explanation. - Delete unused commands/DefenseBoost.java (only callers were already commented out in Binds.java). - Delete duplicate util/constants.java (BotConstants.java is the authoritative copy; this file was unreferenced). - Binds.java: drop the stale commented-out DefenseBoost import and whileTrue binding. acceptEstimate's stale SmartDashboard puts were already cleaned up as part of TDD cycle 4 (VisionFilter extraction) — single putBoolean now. Co-authored-by: Cursor <cursoragent@cursor.com>
…oReset, Intake Six additional test files filling in the matrix of the layered strategy: - BotConstantsTest (Layer B): static config validation. Asserts every CAN ID is unique and in [0, 62], shooter velocity/backspin tables are in plausible bounds at every sampled distance, the two tables share the same distance keys, and no subsystem's stator current limit is 0 or above 200A. - PLogTest (Layer A): captures stdout to lock down the "[Severity] [Category] msg" format across debug/info/unusual/fatal, and verifies fatalException renders the exception's simple name + stack trace. - BaseCamTest (Layer A): in-test FakeCam subclass; verifies the addVisionEstimate(addMeas, filter) contract — no callback on empty estimate, no measurement on filter reject, measurement fires once with stddev on filter accept, and the documented filter-throws behavior (swallow + still feed measurement). - AllianceFindTest (Layer C): DriverStationSim-driven, with HAL.initialize via WpilibTestBase. Asserts setAlliance() swaps all four hub/pass/climb references when DS alliance changes Red <-> Blue and defaults to Blue references on Unknown. - GyroResetTest (Layer D): Mockito-mocked Swerve + Pigeon2; verifies initialize() calls pigeon.setYaw(angle) for default and explicit angle, and isFinished() is true (one-shot). - IntakeSimTest (Layer C): smoke test that simulationPeriodic + periodic survive 20 ticks without exceptions and update cached pivot/roller fields to finite values. Doesn't assert specific physics because Phoenix6 status signals don't propagate sim state cleanly in headless JUnit. Co-authored-by: Cursor <cursoragent@cursor.com>
- Single job on ubuntu-latest, JDK 17 (Temurin) to match WPILib 2026. - Caches ~/.gradle so subsequent runs reuse the WPILib + Phoenix6 native maven artifacts. - Runs ./gradlew build (compileJava + test + everything else). - Uploads the gradle test report HTML + raw test-results XML as an artifact on failure so PR authors can inspect failures without reproducing locally. Co-authored-by: Cursor <cursoragent@cursor.com>
sjqtentacles
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Post-season cleanup pass over the codebase. Eleven concrete bugs fixed, every fix protected by a failing-test-first regression test, plus broader coverage for static config / pure-logic seams that previously had none. Twelve atomic commits, all green on
./gradlew buildin 8 s.Bugs fixed (with test that pins each)
Field.climb_tower_red.y = 5201(off by 1000×; should be5.201— red autos usingalignToClimb()would drive ~5 km off field)FieldBoundsTest"...Bravo..."silently loaded"...Alpha..."auto filesAutoChooserConsistencyTestSwerve.acceptEstimaterejected fast positive rotations only (omega > 1.5, not|omega| > 1.5); now sign-insensitive inVisionFilterVisionFilterTestShootCommand.isAtspeedwas a sticky-once-true boolean; replaced with per-loop hysteresis (enterTol=0.95,exitTol=1.5) in newShooterControlPolicyShooterControlPolicyTestShooter.getRollerVelocity()averagedleader + opposed_follower→ cancelled to ~0 at speed → indexer never tripped its at-speed thresholdShooterRollerVelocityTestHopper.get().Stop()returned aCommandthat 5 callers silently discarded → motor never stopped. Added voidHopper.stop(); updated all in-code callersHopperStopContractTestSwerve.targetPosecachedField.Alliance_Find.climb_towerat construction time → permanently blue alliance. WrappedalignToClimb()inCommands.deferAlignToClimbTestIntake.SlotZeroConfigIntake()at robot init (already applied as part of full config) — removed; method marked@Deprecatedcommands/DefenseBoost.java,util/constants.java(duplicate ofBotConstants)SmartDashboard.putBoolean("Accepted", false)writes interleaved inacceptEstimate→ misleading dashboardRobot.setBrownoutVoltage(6.0)was a deliberate deviation from WPILib's 6.75 V default — kept but added an explanatory commentNew supporting refactors
NamedCommandRegistry— single source of truth for PathPlanner named commands;Robot.Robot()now just callsAuto.initialize(NamedCommandRegistry.all()).NamedCommandConsistencyTestwalks every.autoJSON and asserts every referenced named command + path file exists.VisionFilter— pure-logic AprilTag accept/reject + stddev calculation, extracted fromSwerve.acceptEstimate.ShooterControlPolicy— pure-logic flywheel-target + hysteresis-fire decision, extracted fromShootCommand.Shooter.aggregateRollerVelocity(leader, follower)— small static seam so the leader-only aggregation can be tested without Phoenix6 sim.Test matrix
66 tests, 0 failures, 0 errors, 0 skipped — ~3.4 s of test wall-clock with parallel forks.
VisionFilterTest(10),ShooterControlPolicyTest(12),ShooterRollerVelocityTest(5),PLogTest(5),BaseCamTest(4)FieldBoundsTest(9),AutoChooserConsistencyTest(2),NamedCommandConsistencyTest(2),BotConstantsTest(6),HopperStopContractTest(2),AlignToClimbTest(2)simulationPeriodicAllianceFindTest(3),IntakeSimTest(1)GyroResetTest(3)src/test/java/frc/robot/testutil/WpilibTestBase.javais the shared HAL setup base class — Layer-A tests deliberately do not extend it so the inner loop stays sub-second.Infra
build.gradle: addedassertj-core,mockito-core,mockito-junit-jupiter. Enabled parallel test forks (maxParallelForks = CPUs/2,forkEvery = 50)..github/workflows/build.yml: runs./gradlew buildon push + PR with JDK 17 (Temurin) to match WPILib 2026. Caches~/.gradle. Uploads gradle test report + raw XML on failure.gradlew: made executable (100644 → 100755).Commit history (TDD discipline)
Each TDD cycle is one commit containing both the failing test and the fix —
git show <sha>for any of these shows both the bug-pinning test and the targeted source change side-by-side.Notable trade-offs
Shooter.aggregateRollerVelocityextraction (cycle 6): the originally planned Phoenix6-sim integration test couldn't reliably propagatesetRotorVelocityinjections through the StatusSignal pipeline in headless JUnit (status signals require the device manager loop to tick, which isn't available outside the rio). Extracted the averaging into a tiny static helper and tested it at Layer A — equivalent regression coverage, no flake risk. Commitf4151b0documents this in detail.Hopper.Stop()(capital S, returnsCommand) preserved for legitimate use assetDefaultCommand(...)inBinds.HopperStopContractTestenforces that every other caller uses the new voidstop().Robot.java. Revisit if you see Rio resets in matches.Behavior changes that need a real-robot check
The four fixes below change runtime behavior. The other seven are pure cleanups / no-ops.
|0 - target| < 0.95, always false).alignToClimb()targets the red tower on red instead of always blue.Shooter.Stop(),ShootCommand.end(), and friends now stop the hopper motor synchronously instead of relying on a discarded Command.Recommend bench-testing #1 + #4 specifically — those are the ones most likely to surprise you on the practice field.
Test plan
.github/workflows/build.ymlruns the same./gradlew buildthat's green locally).alignToClimbdrives to the red tower.