Skip to content

Fix broken Ponder build dependency and add PlayerRecord/ExperienceCalculator unit tests - #145

Merged
dmccoystephenson merged 2 commits into
mainfrom
feature/playerrecord-experience-unit-tests
Aug 2, 2026
Merged

Fix broken Ponder build dependency and add PlayerRecord/ExperienceCalculator unit tests#145
dmccoystephenson merged 2 commits into
mainfrom
feature/playerrecord-experience-unit-tests

Conversation

@dmccoystephenson

@dmccoystephenson dmccoystephenson commented Aug 2, 2026

Copy link
Copy Markdown
Member

Summary

This PR does two coupled things — the second needed the first to actually verify in CI:

  1. Fixes the build. pom.xml pinned preponderous:ponder:v0.14-alpha-2, which is broken two ways: that tag doesn't exist in Dans-Plugins/Ponder anymore, and the groupId:artifactId casing (preponderous:ponder) was never resolvable via jitpack (Ponder's own pom.xml declares preponderous:Ponder, and jitpack in practice only serves it under com.github.Dans-Plugins:Ponder). This has been breaking mvn compile/CI on main for everyone (confirmed: the last CI run on main failed with the same error, and none of the other 8 open PRs in this repo have any CI checks reported at all). Switches to com.github.Dans-Plugins:Ponder:v0.14 — the nearest existing tag whose package layout matches this project's existing imports exactly, so no source changes were needed. Related to Update Ponder dependency to latest release #116 but does not close it — see the comment I left there on why a true "latest release" upgrade is a separate, larger, breaking migration.
  2. Bootstraps this repo's first src/test tests, despite JUnit 4 + Mockito already being wired into pom.xml with nothing using them:
    • PlayerRecordTest: skill-level get/set, experience get/set, getKnownSkills/isKnown, getOverallSkillLevel (filters inactive/removed skills), incrementExperience's no-level-up and max-level-skip branches, checkForLevelUp below-threshold behavior, and a save()/load() round-trip.
    • ExperienceCalculatorTest: the getExperienceRequiredForLevelUp formula (level 0, level 1, zero factor, general scaling, int-truncation).
    • No production code behavior was changed by the test PR itself — this is characterization coverage (Stage B of the dev loop: unit-test expansion).

Known gap, left uncovered on purpose: PlayerRecord.learnSkill(...)/levelUp(...) call Bukkit.getPlayer(...) (a static Bukkit accessor). The project pins mockito-core:3.12.4 with no mockito-inline, so stubbing that static call isn't available without adding a new test dependency — left out of scope here. Auto-learning an unknown skill and the leveling-up branch of checkForLevelUp remain covered only by the manual .testcontainer Spigot smoke harness.

Test plan

  • mvn clean package (the exact command CI runs) — green locally after the dependency fix: BUILD SUCCESS, all 18 new tests pass.
  • mvn testTests run: 18, Failures: 0, Errors: 0, Skipped: 0.
  • Verified via a scratch clone of the Ponder repo at both v0.14 and v0.15-alpha-2 that v0.14's package layout matches this project's imports exactly, while v0.15-alpha-2 renames packages (documented in the commit message and issue Update Ponder dependency to latest release #116 comment).

Related: #116 (not closed — see above)


This PR description was drafted during a Gardener session (Stephenson-Software/gardener).

dmccoystephenson and others added 2 commits August 2, 2026 17:01
Characterizes the currently-untested skill-level/experience bookkeeping
logic (level/exp getters-setters, overall level aggregation, level-up
threshold checks, save/load round-trip) using Mockito, and locks in
ExperienceCalculator's requirement formula. Bukkit-static-dependent
branches (auto-learning an unknown skill, leveling up) are left
uncovered since mockito-core (no mockito-inline) can't stub
Bukkit.getPlayer(...).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The pinned tag v0.14-alpha-2 no longer exists in Dans-Plugins/Ponder,
and the declared groupId/artifactId ("preponderous"/"ponder") was
never resolvable via jitpack for this repo. This broke `mvn compile`
for everyone, including CI on main (confirmed failing since at least
the last merge to main).

Switches to com.github.Dans-Plugins:Ponder:v0.14 (jitpack's standard
GitHub-based coordinates), the nearest existing tag whose package
layout (preponderous.ponder.minecraft.{abs,nms,spigot.*}) exactly
matches this project's existing imports, so no source changes were
needed. Related to #116, but not a full fix: the actual latest tag
(v0.15-alpha-2) renames minecraft.spigot.* to minecraft.bukkit.*,
which is a breaking migration left for a follow-up.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dmccoystephenson dmccoystephenson changed the title Add unit tests for PlayerRecord and ExperienceCalculator Fix broken Ponder build dependency and add PlayerRecord/ExperienceCalculator unit tests Aug 2, 2026
@dmccoystephenson

dmccoystephenson commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

Self-review rubric:

  • Scope: PASS — every changed file (pom.xml, CHANGELOG.md, 2 new test files) is necessary for one of the two things this PR does (unblock the build, add characterization tests); no unrelated formatting/renames/comment churn.
  • Tests-new: N/A (PASS) — no new production public methods were added; the PR itself is the new test coverage (18 tests) for existing PlayerRecord/ExperienceCalculator methods.
  • Tests-fix (empirical): PASS — for the pom.xml dependency fix, the pre-fix pom.xml was checked out (git checkout a29fb0d~1 -- pom.xml) and mvn compile was run: confirmed FAIL (Could not resolve dependencies ... preponderous:ponder:jar:v0.14-alpha-2). The fixed pom.xml was restored and mvn test was run: confirmed PASS (Tests run: 18, Failures: 0, BUILD SUCCESS). Not scored from reasoning alone.
  • Sibling structure: PASS — new test files mirror their production package layout (src/test/java/dansplugins/simpleskills/{playerrecord,experience}/...Test.java), matching this project's existing src/main structure; no sibling test files existed yet to diverge from.
  • Sibling renames: N/A — no renames in this PR.
  • Docs: PASSCHANGELOG.md has [Unreleased] entries for both changes. HelpCommand.java, COMMANDS.md, USER_GUIDE.md, CONFIG.md correctly untouched — no command, permission, or config-key behavior changed.
  • Issue resolution: PASS — no Closes #N claimed. Update Ponder dependency to latest release #116 is referenced but deliberately not closed (commented on the issue explaining the fix here is a minimal "restore a working build" patch, not the "upgrade to latest release" the issue actually asks for — that's a separate, breaking, larger migration).
  • CI: PASS — green on the PR head SHA (gh pr checks 145), confirmed after the dependency fix was pushed.

Repo-specific:

  • Skill/stat parity: N/A (no signal)StatsCommand.java/TopCommand.java/SkillCommand.java not touched by this PR.
  • Testcontainer harness untouched implies UNVERIFIED, not PASS: correctly flagged — this PR's PlayerRecordTest explicitly documents (in its class Javadoc and the PR body) that learnSkill/levelUp's live-Bukkit.getPlayer(...) branches are not exercised by these unit tests (mockito-core has no mockito-inline for static mocking) and remain covered only by the manual .testcontainer Spigot smoke harness. Not claimed as tested.

Note for the merge gate: this PR modifies pom.xml, which is on this repo's do-not-auto-merge list (dependency changes affect the shaded JAR/release artifact). It is being left open for human review/merge rather than merged autonomously, even though CI is green and the rubric above passes.


This comment was drafted during a Gardener session (Stephenson-Software/gardener).

@dmccoystephenson
dmccoystephenson merged commit 4ca1834 into main Aug 2, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant