Repository navigation
test(bot): split oversized queuemanipulation spec file - #1730
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR splits the oversized ChangesQueue Manipulation Test Suite Split
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Failed to generate code suggestions for PR |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bot/src/utils/music/queueManipulation.replenish.spec.ts (1)
1-1255: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile exceeds the 500-line target set by the linked issue.
At ~1255 lines, this is the largest of the three files reviewed and the furthest from the "under 500 lines" acceptance criterion in the PR objectives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/utils/music/queueManipulation.replenish.spec.ts` around lines 1 - 1255, This spec file is far over the 500-line target, so reduce its size by moving groups of related replenishQueue scenarios into separate spec files or shared test helpers. Keep the key setup in queueManipulation.replenish.spec.ts, and preserve coverage by extracting repeated mocks/queue builders around replenishQueue, createQueueMock, and replenishWithSingleCandidate into reusable fixtures.packages/bot/src/utils/music/queueManipulation.autoplay.spec.ts (1)
1-1005: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile exceeds the 500-line target set by the linked issue.
This file is ~1000 lines, well past the "under 500 lines" acceptance criterion for the split (per PR objectives). Consider splitting further, e.g. separating VC-blend/multi-user scoring scenarios from the fallback-candidate/jitter/genre-collection scenarios.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/utils/music/queueManipulation.autoplay.spec.ts` around lines 1 - 1005, The spec file is far over the 500-line target and needs to be split into smaller test files. Move related cases out of queueManipulation.autoplay.spec.ts into separate specs grouped by behavior, using the existing describe blocks like replenishQueue fallback/jitter/genre collection, multi-user VC blend, and buildVcContributionWeights to guide the split. Keep the shared mocks/helpers centralized where possible, but reorganize the tests so each file stays comfortably under the acceptance threshold.
🧹 Nitpick comments (3)
packages/bot/src/utils/music/queueManipulation.priority.spec.ts (1)
684-816: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnit tests for
getGenreFamilies/calculateGenreFamilyPenalty/enrichWithAudioFeaturesare nested inside the "diversity improvements"describe, inheriting an unrelatedbeforeEachthat mocksreplenishQueuecollaborators these tests never touch.Purely cosmetic — the tests still pass — but placing pure-function unit tests inside a
describewhosebeforeEachexists solely to supportreplenishQueueintegration tests is a bit confusing when scanning the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/utils/music/queueManipulation.priority.spec.ts` around lines 684 - 816, Move the pure-function test blocks for getGenreFamilies, calculateGenreFamilyPenalty, and enrichWithAudioFeatures out of the “diversity improvements” describe so they do not inherit the replenishQueue-focused beforeEach. Keep those tests in their own top-level describe (or sibling describe) near the related helpers, using the same symbols to make the file easier to scan and avoid implying they depend on the queue-mocking setup.packages/bot/src/utils/music/queueManipulation.autoplay.spec.ts (2)
11-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMock boilerplate and
createQueueMockare duplicated verbatim across the split spec files.The jest.mock setup for
lru-cache,discord-player,@lucky/shared/*,../../lastfm,../../spotify/*, and thecreateQueueMockhelper are copy-pasted identically across this file,queueManipulation.priority.spec.ts, andqueueManipulation.replenish.spec.ts(and presumably the other two split files). Any future change toqueueManipulation's dependency surface now needs to be replicated in five places, which works against the maintainability goal that motivated this split.Consider extracting the shared mocks and
createQueueMockinto a common test helper module (e.g.queueManipulation.testUtils.ts) imported by all five spec files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/utils/music/queueManipulation.autoplay.spec.ts` around lines 11 - 172, The shared test setup and `createQueueMock` in `queueManipulation.autoplay.spec.ts` are duplicated across the split queueManipulation specs, so consolidate them into a common helper module and import it from each file. Move the repeated `jest.mock` blocks for `lru-cache`, `discord-player`, `@lucky/shared/*`, `../../lastfm`, `../../spotify/*`, and the `createQueueMock` helper into a shared test utility such as `queueManipulation.testUtils.ts`, then update this spec and the other queueManipulation spec files to reuse those exports instead of redefining them.
408-451: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeveral assertions are effectively no-ops.
expect(addedTracks.length).toBeGreaterThanOrEqual(0)(Lines 447, 547) is always true and doesn't verify any behavior; likewise theif (addedTracks.length > 0) {...}guard pattern used elsewhere in this suite means the assertion body can be skipped entirely without failing the test. These tests provide little regression protection for the multi-user blend/enrichment paths they claim to cover.Also applies to: 503-548
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/utils/music/queueManipulation.autoplay.spec.ts` around lines 408 - 451, The assertions in this autoplay queue test are no-ops and don’t verify the multi-user blend path. Update `replenishQueue` coverage by asserting concrete behavior on `addedTracks` and the mocked seed consumers (`consumeBlendedSeedSliceMock`, `consumeSingleUserSeedSliceMock`) instead of using always-true checks or optional `if` guards. Make the test fail when no track is added for the expected scenario, and assert the expected track shape/content directly so the `adapts seed consumption based on VC member Last.fm linkage` case actually validates behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bot/src/utils/music/queueManipulation.priority.spec.ts`:
- Around line 1-816: The priority spec file is too large and needs to be split
to meet the under-500-line requirement. Move related test groups out of
queueManipulation.priority.spec.ts into focused spec files, keeping the existing
describe blocks for replenishQueue, getGenreFamilies,
calculateGenreFamilyPenalty, and enrichWithAudioFeatures together only if
needed. Preserve the current test behavior by extracting shared mocks/helpers
like createQueueMock and the service mock setup into reusable test utilities or
a common fixture module, so each new spec remains easy to locate and maintain.
---
Outside diff comments:
In `@packages/bot/src/utils/music/queueManipulation.autoplay.spec.ts`:
- Around line 1-1005: The spec file is far over the 500-line target and needs to
be split into smaller test files. Move related cases out of
queueManipulation.autoplay.spec.ts into separate specs grouped by behavior,
using the existing describe blocks like replenishQueue fallback/jitter/genre
collection, multi-user VC blend, and buildVcContributionWeights to guide the
split. Keep the shared mocks/helpers centralized where possible, but reorganize
the tests so each file stays comfortably under the acceptance threshold.
In `@packages/bot/src/utils/music/queueManipulation.replenish.spec.ts`:
- Around line 1-1255: This spec file is far over the 500-line target, so reduce
its size by moving groups of related replenishQueue scenarios into separate spec
files or shared test helpers. Keep the key setup in
queueManipulation.replenish.spec.ts, and preserve coverage by extracting
repeated mocks/queue builders around replenishQueue, createQueueMock, and
replenishWithSingleCandidate into reusable fixtures.
---
Nitpick comments:
In `@packages/bot/src/utils/music/queueManipulation.autoplay.spec.ts`:
- Around line 11-172: The shared test setup and `createQueueMock` in
`queueManipulation.autoplay.spec.ts` are duplicated across the split
queueManipulation specs, so consolidate them into a common helper module and
import it from each file. Move the repeated `jest.mock` blocks for `lru-cache`,
`discord-player`, `@lucky/shared/*`, `../../lastfm`, `../../spotify/*`, and the
`createQueueMock` helper into a shared test utility such as
`queueManipulation.testUtils.ts`, then update this spec and the other
queueManipulation spec files to reuse those exports instead of redefining them.
- Around line 408-451: The assertions in this autoplay queue test are no-ops and
don’t verify the multi-user blend path. Update `replenishQueue` coverage by
asserting concrete behavior on `addedTracks` and the mocked seed consumers
(`consumeBlendedSeedSliceMock`, `consumeSingleUserSeedSliceMock`) instead of
using always-true checks or optional `if` guards. Make the test fail when no
track is added for the expected scenario, and assert the expected track
shape/content directly so the `adapts seed consumption based on VC member
Last.fm linkage` case actually validates behavior.
In `@packages/bot/src/utils/music/queueManipulation.priority.spec.ts`:
- Around line 684-816: Move the pure-function test blocks for getGenreFamilies,
calculateGenreFamilyPenalty, and enrichWithAudioFeatures out of the “diversity
improvements” describe so they do not inherit the replenishQueue-focused
beforeEach. Keep those tests in their own top-level describe (or sibling
describe) near the related helpers, using the same symbols to make the file
easier to scan and avoid implying they depend on the queue-mocking setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6700d7a-cf0e-41f0-b404-a4d1ce8a0f05
📒 Files selected for processing (6)
packages/bot/src/utils/music/queueManipulation.autoplay.spec.tspackages/bot/src/utils/music/queueManipulation.dedup.spec.tspackages/bot/src/utils/music/queueManipulation.operations.spec.tspackages/bot/src/utils/music/queueManipulation.priority.spec.tspackages/bot/src/utils/music/queueManipulation.replenish.spec.tspackages/bot/src/utils/music/queueManipulation.spec.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
LucasSantana-Dev
enabled auto-merge (squash)
July 9, 2026 08:20
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Auto-approved: Refactors a monolithic test file into six focused test files. No production code changes; all 79 tests preserved. Low risk.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Test-only refactor: splits a large spec file into focused modules by concern. No production code or behavior changes, low risk.
Re-trigger cubic
Split the monolithic 3248-line queueManipulation.spec.ts into 5 focused test files organized by concern: operations (shuffle, move, remove, rescue), deduplication logic, main replenish tests, advanced autoplay scenarios, and Spotify priority/diversity. Preserves all 79 tests with natural boundaries based on describe blocks and exported functions. Closes #1618
- Remove unused QueryType variable from queueManipulation.dedup.spec.ts - Fix weak test assertion in autoplay.spec.ts (toBeGreaterThanOrEqual → toBeGreaterThan) - Unwrap conditional test assertions in priority.spec.ts to ensure they always run - Extract scoring-related tests into new queueManipulation.scoring.spec.ts: - getGenreFamilies tests - calculateGenreFamilyPenalty tests - enrichWithAudioFeatures tests This reduces priority.spec.ts from 816 to 683 lines, moving closer to the 500-line target.
Ports the same fix from #1741 into the split-out spec file.
The functions calculateGenreFamilyPenalty, enrichWithAudioFeatures, and getGenreFamilies were moved to queueManipulation.scoring.spec.ts and are no longer used in priority.spec.ts. Removed the unused imports to resolve CodeQL finding.
- scoring.spec.ts: add missing @lucky/shared/services mocks to prevent prismaClient.ts import.meta error. Mocks placed before imports to prevent actual service modules from being loaded. - priority.spec.ts: revert test assertion regression from strict expect to original graceful if-check for "applies Spanish/Latin genre penalty" test to match pre-split behavior. All 84 queueManipulation tests pass. Bot test suite: 2852/2853 tests.
LucasSantana-Dev
force-pushed
the
fix-1618-split-queue-spec
branch
from
July 10, 2026 16:19
acf8411 to
0f80046
Compare
|
This was referenced Sep 4, 2026
LucasSantana-Dev
added a commit
that referenced
this pull request
Sep 4, 2026
…#2178) ## Summary Three unrelated CI hygiene fixes bundled into one PR since they all touch `.github`/root CI plumbing and were filed together by the same backlog run. - Fixes the `test:music:incident` regex so it no longer silently skips the queue-manipulation and player-factory regression suites. - Adds `timeout-minutes` to every job across all 19 workflow files so a hang can't burn the 360-minute GitHub default. - Deletes the unreferenced, drifted root `jest.config.cjs`. Closes #2154 Closes #2162 Closes #2163 ## #2154 - test:music:incident regex `queueManipulation.spec` was split by #1730 into `queueManipulation.{autoplay,dedup,operations,priority,replenish,scoring}.spec.ts`, and `playerFactory.test` never matched because the real file is `playerFactory.spec.ts`. Both branches matched zero files, so those two regression guards ran 0 tests with a green check. Fix: `queueManipulation.spec` -> `queueManipulation\..*spec`, `playerFactory.test` -> `playerFactory\.spec`. Verification, every branch matches at least one file: ``` $ find packages/bot/src -name '*.ts' | grep -Ec 'engineManager.spec' 1 $ find packages/bot/src -name '*.ts' | grep -Ec 'autoplay.spec' 2 $ find packages/bot/src -name '*.ts' | grep -Ec 'queueManipulation\..*spec' 6 $ find packages/bot/src -name '*.ts' | grep -Ec 'playerFactory\.spec' 1 $ find packages/bot/src -name '*.ts' | grep -Ec 'play/index.spec' 1 ``` Full `npm run test:music:incident` run (after `npm run db:generate`, which CI's `test-bot` job already runs via the `build-shared` dependency chain): ``` Test Suites: 10 passed, 10 total Tests: 135 passed, 135 total Snapshots: 0 total Time: 2.971 s ``` 10 suites now run (up from 8 matching before the fix, 2 of which - `queueManipulation.spec` and `playerFactory.test` - matched 0 files). ## #2162 - timeout-minutes on every job No job in any of the (now 19, was 18 when the issue was filed) workflow files under `.github/workflows/` set `timeout-minutes`; every job fell back to GitHub's 360-minute cap. Sized from `gh run list --workflow <file> --limit 20` history (run-level durations via `startedAt`/`updatedAt`, and per-job durations for `ci.yml` via `gh run view --json jobs`), using roughly 2x observed p95 rounded up, with floors of 15 for test/lint jobs, 30 for docker builds, 30 for deploy/webhook-wait jobs, and 10 for trivial label/notify jobs: | workflow | job | timeout-minutes | basis | |---|---|---|---| | ci.yml | build-shared | 15 | p95 5.4min, test/lint floor | | ci.yml | checks | 15 | p95 7.4min | | ci.yml | test-shared | 15 | p95 5.0min | | ci.yml | test-backend | 15 | p95 6.7min | | ci.yml | test-bot | 15 | p95 7.0min | | ci.yml | test-youtube-smoke | 15 | conditional, same class as other test jobs | | ci.yml | test-frontend | 15 | p95 6.8min | | ci.yml | madge | 20 | p95 9.1min, 2x rounded up | | ci.yml | detect-docker-changes | 10 | p95 0.3min, trivial | | ci.yml | docker-build | 60 | matrix job runs up to 5 sequential build attempts on retry (issue #2015's BuildKit race); observed real max 43.2min (bot leg), 33.9min (backend leg) | | ci.yml | docker-build-check | 10 | p95 0.1min, trivial assert | | ci.yml | quality-gate | 10 | p95 0.1min, trivial assert | | ci.yml | sonar | 15 | p95 2.4min, has a retry-once path | | ci.yml | security | 25 | p95 11.5min, 2x rounded up | | auto-update-pr-branches.yml | update | 10 | p95 0.9min, trivial | | bundle-size.yml | size-limit | 15 | p95 5.7min | | deploy-frontend-cf.yml | deploy | 30 | deploy floor | | deploy-staging.yml | deploy-staging | 30 | deploy floor | | deploy.yml | deploy | 45 | p95 21.3min; job has webhook-wait polling loops the issue calls out by name | | destructive-interaction-gate.yml | destructive-interaction-gate | 15 | p95 3.4min | | docker-publish.yml | build-and-push | 30 | docker build floor (p95 11.7min) | | external-pr-notify.yml | notify | 10 | p95 1.9min, trivial notify | | external-pr-notify.yml | digest | 10 | p95 1.9min, trivial notify | | fork-ci-autoapprove.yml | auto-approve | 10 | p95 2.9min, trivial | | migration-gate.yml | migrate-gate | 15 | p95 3.4min | | mutation.yml | mutation | 30 | p95 13.3min, 2x rounded up | | pr-agent.yml | pr-agent | 10 | p95 1.2min, trivial | | pr-labels.yml | label | 10 | p95 4.1min, trivial label job | | release-please.yml | release-please | 10 | p95 0.5min, trivial | | release-tag-guard.yml | ensure-release-tag | 10 | p95 0.5min (max outlier 3.75min), trivial | | stale.yml | stale | 10 | p95 0.3min, trivial | Not changed - `quality.yml`'s `quality` job and `review-tools.yml`'s `danger` job both call a reusable workflow via `uses:`. GitHub Actions does not support `timeout-minutes` on a job that calls a reusable workflow (only `name`, `uses`, `with`, `secrets`, `needs`, `if`, `permissions`, `strategy` are valid there), so these two are left as-is. Verification - every workflow file still parses and actionlint reports zero issues: ``` $ python3 -c "import yaml,glob;[yaml.safe_load(open(f)) for f in glob.glob('.github/workflows/*.yml')]" (no output - all 19 files parsed) $ actionlint (no output - zero findings) ``` `git diff --stat` on the workflow files confirms only the `timeout-minutes` lines were added, no reformatting: ``` 17 files changed, 31 insertions(+) ``` ## #2163 - stale root jest.config.cjs Confirmed nothing references the root `jest.config.cjs` (no root `jest` script exists either, only the `jest` devDependency entry): ``` $ grep -rn "jest.config.cjs" --include=package.json --include=*.yml --include=*.sh --include=*.md . | grep -v node_modules | grep -v packages/ (no matches) ``` All other `jest.config.cjs` mentions found in the repo (CHANGELOG.md, docs/TESTING.md, decisions/*.md) refer to the package-scoped configs (`packages/backend/jest.config.cjs`, `packages/bot/jest.config.cjs`, `packages/shared/jest.config.cjs`), not the root one. No doc instructs running bare `jest` from the repo root, so nothing needed updating there. Deleted the file. ## Verification - `npm run test:music:incident` - 10/10 suites, 135/135 tests passing. - `npm run type:check` - clean across shared/bot/backend/frontend. - `python3 -c "import yaml,glob;..."` and `actionlint` - all 19 workflow files parse, zero lint findings. - `git diff --stat` on touched workflow files - additions only, no reformatting. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Fixes #2154, #2162, and #2163: the music incident check now matches all relevant suites, eligible GitHub Actions jobs have bounded runtimes, and the unused root Jest config is removed. This prevents false-green checks, limits hung jobs, and removes stale configuration. **Details** - Matches both `playerFactory.spec` and `playerFactory.test`, plus split `queueManipulation` suites. - Adds 10–75 minute timeouts to eligible jobs across all 19 workflow files; reusable workflow callers remain unchanged because GitHub does not support job timeouts there. - Uses 75 minutes for deployment polling and 20 minutes for the sequential PR review workflow. - Verified the incident tests pass, all workflow files parse, and `actionlint` reports no issues. <sup>Written for commit ea04bd8. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2178?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
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
2 participants
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
Split the monolithic 3248-line
queueManipulation.spec.tsinto 5 focused test files organized by concern:All 79 tests preserved with natural boundaries based on describe blocks and exported functions.
Closes #1618
Summary by cubic
Split the 3,252-line
queueManipulation.spec.tsinto six focused test files (operations, dedup, replenish, autoplay, priority, scoring) to improve readability and maintainability. All 84 tests pass; no behavior changes; closes #1618.queueManipulation.scoring.spec.ts, trimming the priority spec.QueryTypein dedup, tightened an autoplay assertion (>), and usedjest.resetAllMocks()in the multi-user VC blend suite.@lucky/shared/servicesmocks in scoring to avoid prisma import issues, and restored one priority test’s conditional check to match pre-split behavior.Written for commit 0f80046. Summary will update on new commits.
Summary by CodeRabbit