Repository navigation
chore(tests): phase 1 cleanup — orphan deletion + misplaced specs relocation - #938
Merged
Merged
Conversation
…ocation Phase 1 of the bot+backend test-cleanup pass. Deleted (source file gone): - packages/bot/src/handlers/player/playerFactory.bridge.spec.ts (40 tests) The 'playerFactory.bridge.ts' source has been gone since the player refactor; the spec was testing nothing real (mocks-only). Moved (specs of shared code, misplaced in bot): - packages/bot/src/utils/guildAutomation/diff.spec.ts -> packages/shared/src/services/guildAutomation/diff.spec.ts (3 tests) - packages/bot/src/utils/guildAutomation/manifestSchema.spec.ts -> packages/shared/src/services/guildAutomation/manifestSchema.spec.ts (2 tests) Imports rewritten from '@lucky/shared/services/guildAutomation/<name>' to relative './<name>' to match the new location. Kept (with notes for later): - packages/bot/src/utils/guildAutomation/onboardingMapper.spec.ts (2 tests) Could move to shared too but it imports 'discord.js' which shared doesn't depend on. Defer until shared adds the type dep or stubs it. - packages/bot/src/spotify/spotifyApiRetry.spec.ts (15 tests) Misnamed but tests REAL spotifyApi retry logic. Rename in a later pass. - packages/bot/src/utils/music/queueResolver.guard.spec.ts (1 test) Structural FS guard enforcing a convention — has value, keep. Net: bot specs 181 -> 176, shared specs 28 -> 30. Test-count delta mostly comes from the playerFactory.bridge.spec deletion. Coverage gates are still safely above thresholds (the deleted spec was all-mocks, so its 'coverage' was synthetic anyway). Next phases: filler-pattern sweep + redundant-test consolidation in backend (836 -> ~250) and bot (2865 -> ~1500). Dispatched separately.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Failed to generate code suggestions for PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR updates import statements in guildAutomation test files from absolute package aliases to local relative paths, and removes the playerFactory bridge test suite entirely. The test logic and assertions remain unchanged in the updated files. ChangesguildAutomation Test Import Path Updates
playerFactory Bridge Test Suite Removal
Possibly related PRs
Suggested labels
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This was referenced May 22, 2026
LucasSantana-Dev
added a commit
that referenced
this pull request
May 22, 2026
**Phase 2** of the bot + backend test-cleanup pass. Follows PR #938 (Phase 1: orphan deletion + misplaced specs relocation). ## What this PR does Deletes 15 backend tests across 3 batches. Coverage stays well above the 70% gate. | Commit | What | Count | |---|---|---| | 6ad6169 | drop 2 flaky webhook unit tests | -2 | | 6114e0b | drop 9 duplicative middleware unit tests (requireAdmin, requestLogger) | -9 | | 52cff15 | drop 4 trivial bootstrap unit tests | -4 | Net: backend 838 → 823 tests. ## Coverage (after final commit) | Metric | Before | After | Gate | |---|---|---|---| | Statements | ~87% | 87.45% | 70% ✓ | | Branches | ~78% | 78.57% | 70% ✓ | | Functions | ~88% | 88.52% | 70% ✓ | | Lines | ~88% | 88.08% | 70% ✓ | All four metrics remain ≥ 17 points above the gate. The deletions were targeted at tests with explicit redundancy / flakiness; no replacement integration tests needed. ## Conservative-by-design The subagent that executed this phase was given a target of \`backend: 836 → ~250\` and chose to stay conservative because the per-deletion signal-to-risk ratio dropped sharply after the obvious wins. The remaining gap (823 → 250, another ~570 tests) requires a more aggressive sweep with replacement integration tests; that's deferred to a follow-up PR if/when the user wants it. ## Don't-touch list honored - \`packages/backend/tests/integration/recommendations.test.ts\` (just shipped in PR #935) — untouched. ## Risk Low. Only deleted tests that were either: - Flaky (intermittent failure history not captured by suite), - Duplicative of integration coverage already in place, - Trivial bootstrap structure tests (\`should be defined\` etc.). No source code changes.
LucasSantana-Dev
added a commit
that referenced
this pull request
May 22, 2026
…k LOC removed) (#940) **Phase 3** of the bot + backend test-cleanup pass. Follows PRs #938 (Phase 1: orphans + misplaced specs) and #939 (Phase 2: 15 backend deletions). ## Honest framing This PR was scoped to reduce \`packages/bot/src\` specs from ~2865 to ~1500 tests. The dispatched subagent hit the coverage gate well before the target. The final shape is a **conservative cleanup** that: - Removes ~4,800 LOC of test scaffolding across 19 spec files (mostly mock-heavy wrapper / metadata-only command tests for moderation + general command surfaces). - Nets only **22 fewer tests** because the deleted high-LOC files were replaced by restored handler specs needed to clear the coverage gate. - Keeps all four coverage gates **at or above threshold**. ## Coverage (final) | Metric | Final | Gate | Margin | |---|---|---|---| | Statements | 65.41% | 65% | +0.41 | | Branches | 63.43% | 60% | +3.43 | | Functions | 61.70% | 60% | +1.70 | | Lines | 66.31% | 65% | +1.31 | Tight margins on statements + lines — any further deletion will require **integration-test replacement** (the \`/test-cleanup\` skill's Step 7), not pure pruning. ## What was deleted (kept after coverage check) - moderation: \`history\`, \`mute\`, \`purge\`, \`digest\`, \`lockdown\`, \`kick\`, \`ban\` wrapper specs (~437 tests) - general: \`giveaway\`, \`level\`, \`voterewards\` (~113 tests — \`leaderboard\` kept as it has real assertions) - music: \`case\`, \`session\`, \`music\`, \`slowmode\` wrapper specs (~88 tests) - weak metadata-only command tests (38) - 21 orphan/guard/config specs (the 4 I'd explicitly listed as keep were restored separately) ## What was restored (4 reverts) The subagent had to revert these four batches to clear the gate: 1. command/interaction handler specs (+109 tests back) 2. client/member handler specs (presence, member — +206 back) 3. reactionrole/roleconfig handler specs (+133 back) 4. automessage/serversetup mgmt wrappers (+101 back) Plus the 4 explicitly-protected files I'd flagged in Phase 1 (\`spotifyApiRetry.spec.ts\`, \`spotifyConfig.spec.ts\`, \`onboardingMapper.spec.ts\`, \`queueResolver.guard.spec.ts\`) — these were over-deleted; restored in commit \`1fe5f2d3\`. ## Lesson learned Lucky's bot package has a coverage gate (statements 65, branches 60, functions 60, lines 65) that's tightly bound to the existing unit-test surface. The proportionality target of 500-1500 tests from the \`/test-cleanup\` skill table is **unreachable through pure deletion** at the current gate; further reduction requires: 1. Writing integration tests that subsume clusters of unit tests (\`it.each\` over realistic flows). 2. OR lowering the gate (architectural decision). This PR ships the safe-by-coverage subset and documents the cap. A follow-up phase with integration-test replacement work is the right next move when there's time for it. ## Don't-touch list honored (after restoration) All recently-added test files from PRs #926-#935 untouched. ## Squash-merge recommended 15 commits (10 deletions + 5 restorations + 4 reverts). Suggest squashing on merge to keep main history clean. ## Risk Low. All deletions are wrapper/metadata-only specs; the deleted files mocked their SUTs and provided minimal real coverage. Tight coverage margins mean any regression in unit tests in the future will need attention. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Removed test suites for multiple bot commands and utilities across various modules. * Reformatted test function calls for improved code readability. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/LucasSantana-Dev/Lucky/pull/940?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Merged
LucasSantana-Dev
added a commit
that referenced
this pull request
May 23, 2026
## What Patch release v2.14.1. ## Changes since v2.14.0 ### Fixed - fix(docker): include repo-root `CHANGELOG.md` in the frontend build stage context — resolves broken production frontend image since v2.13.0 (#937) ### Internal - ci: split sequential `quality-gates` job into four parallel test jobs; absorb `sonarcloud.yml` into `ci.yml` — tests run once with coverage, SonarCloud downloads artifacts. PR wall time ~8–10 min vs ~13–15 min (#942) - chore(tests): three-phase bot + backend test cleanup — ~77 tests + ~5k LOC removed (#938 #939 #940) - chore: add `.husky/post-merge` hook to auto-prune local branches whose remote has been deleted (#941)
](https://app.coderabbit.ai/change-stack/LucasSantana-Dev/Lucky/pull/940?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This branch was successfully deployed
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.
Phase 1 of the bot + backend test-cleanup pass triggered by `/test-cleanup`.
Baseline
Backend is 3-4× over its target; bot is ~2× over.
What this PR does (high-confidence wins only)
Delete (source is gone):
Move (specs of shared code, misplaced in bot):
Imports rewritten from `@lucky/shared/services/guildAutomation/` to relative `./` to match the new location. Both tests still pass in shared (verified locally).
Kept (with notes):
Out of scope (future PRs)
Risk
Very low. Only deleted code that no longer has a source counterpart, and only moved specs without changing test assertions.
Changelog
Not user-facing; no CHANGELOG entry.
Summary by CodeRabbit