Gandalf/sofarr
Public Access
0
0
Fork 0
Code Issues 1 Pull Requests Actions 1 Packages Projects Releases 59 Wiki Activity

Testing scope Review and Recommendations #60

New Issue
Closed
opened 2026-05-27 23:43:42 +01:00 by Gandalf · 4 comments
Gandalf commented 2026-05-27 23:43:42 +01:00
Owner
Copy Link
Copy Source

Current Coverage Profile

Area Approximate Coverage Key Characteristics
Overall Statements / Lines ~55% Primarily happy-path unit tests
Branch Coverage ~40% Limited exploration of conditional paths
Dashboard Routes ~42% lines / ~25% branches Core data aggregation paths under-tested
Ombi Integration 0% No dedicated integration tests
Background Poller 0% No unit or integration coverage
SSE Streaming Endpoint Excluded Intentionally omitted from test suite
Rate Limiting Mostly skipped Tests bypass production rate limit logic
Frontend UI Components Low Limited coverage of conditional rendering

Technical Analysis of Gaps

1. Absence of Ombi Integration Tests

Technical Requirement:
Ombi integration involves request enrichment (decorateRequestsWithArrLinks), webhook processing, and cross-service linking between Ombi and *arr applications. These flows contain multiple conditional branches based on media type, ID variants, and admin status.

Current Gap:
Zero dedicated integration tests exist for this subsystem despite its complexity and recent changes.

Technical Implication:
Without integration-level tests exercising the full request → enrichment → link generation pipeline, regression protection for this integration is absent. This reduces confidence in changes affecting ID extraction logic (theTvDbId, tvDbId, theMovieDbId, etc.) and admin link generation.

Recommendation:
Create a dedicated integration test suite (tests/integration/ombi.test.js) covering:

  • Request fetching and filtering
  • decorateRequestsWithArrLinks behavior across movie/TV cases
  • Webhook payload processing
  • Link generation for both movie and TV requests under admin and non-admin contexts

2. Untested Background Polling Engine

Technical Requirement:
The poller (server/utils/poller.js) is responsible for periodic data aggregation from multiple external services, managing concurrency via a polling guard, notifying SSE subscribers, and handling graceful shutdown. It forms the backbone of the real-time dashboard behavior.

Current Gap:
No unit or integration tests cover this component.

Technical Implication:
Key behaviors such as concurrent execution prevention, subscriber notification, error recovery paths, and interval-based scheduling remain unverified. This creates a blind spot in testing the system's core data freshness mechanism.

Recommendation:
Implement unit tests for poller.js focusing on:

  • Polling guard logic (polling flag behavior)
  • Subscriber registration and notification (onPollComplete / offPollComplete)
  • Error handling and recovery within pollAllServices
  • Graceful shutdown sequence
  • Behavior under varying POLL_INTERVAL configurations

3. Exclusion of SSE Endpoint from Testing

Technical Requirement:
The /api/dashboard/stream endpoint implements Server-Sent Events for real-time push updates. It is central to the application's stated real-time capabilities and interacts directly with the poller.

Current Gap:
This endpoint is intentionally excluded from the test suite.

Technical Implication:
There is no automated verification of event emission timing, client connection lifecycle, proper cleanup on disconnect, or correct payload formatting for different event types (downloads, history, requests).

Recommendation:
Add integration tests for the SSE endpoint that verify:

  • Successful connection and initial data push
  • Event emission on poll completion
  • Proper handling of multiple concurrent clients
  • Connection cleanup and resource release

4. Limited Coverage of Rate Limiting and Error Scenarios

Technical Requirement:
Production rate limiting and external service error handling (timeouts, 5xx responses, rate limit responses) are critical for system stability when interacting with Sonarr, Radarr, Ombi, and download clients.

Current Gap:
Most integration tests bypass rate limiting via SKIP_RATE_LIMIT. There is minimal coverage of failure modes using nock.

Technical Implication:
The test suite does not exercise the system's resilience characteristics under load or external service degradation.

Recommendation:

  • Remove or minimize use of SKIP_RATE_LIMIT in integration tests
  • Add dedicated test cases using nock for:
    • Rate limit responses (429)
    • Server errors (5xx)
    • Timeouts
    • Partial failures across multiple services

5. Insufficient Frontend Component Coverage

Technical Requirement:
The frontend renders dynamic UI elements based on data properties such as arrType, arrLink, isAdmin, movieName, and seriesName. Conditional rendering logic directly affects user-facing features (e.g., service icons and deep links).

Current Gap:
Coverage of client/src/ui/downloads.js and related components is low, with limited testing of conditional rendering paths.

Technical Implication:
Changes to rendering logic for admin features or media-type differentiation carry higher regression risk without targeted component tests.

Recommendation:
Add Vitest + jsdom tests for:

  • createServiceIcons function and its conditional logic
  • Rendering of Radarr/Sonarr icons based on arrType and arrLink
  • Behavior under admin vs non-admin states
  • Differentiation between movie (movieName) and series (seriesName) content

Recommended Coverage Targets

Based on the architectural characteristics of the system (real-time event-driven flows, multiple external integrations, admin-only features), the following targets are recommended:

Metric Current Recommended Minimum Stretch Target
Statements / Lines ~55% 75% 85%
Branches ~40% 65% 75%
Functions - 70% 80%

These targets should be enforced via CI and reviewed periodically as new features are added.

Prioritized Action Plan

Phase 1 (Immediate)

  1. Create Ombi integration test suite
  2. Add unit tests for the poller component
  3. Implement basic SSE endpoint integration tests

Phase 2 (Short Term)

  1. Expand rate limiting and error scenario coverage
  2. Increase frontend component test coverage for downloads UI
  3. Raise CI coverage thresholds to recommended minimums

Phase 3 (Ongoing)

  1. Maintain test coverage during feature development
  2. Add contract-style tests for external service interactions (future consideration)
  3. Review and update test strategy quarterly as the system evolves

Conclusion

Effective test coverage for this system requires deliberate focus on the real-time, integration-heavy, and conditionally-rendered aspects of the architecture. The recommendations above are designed to align test scope with the technical requirements of background processing, external service resilience, and dynamic UI behavior.

Implementation of these improvements will provide stronger regression protection and greater confidence in the correctness of core system behaviors.

## Current Coverage Profile | Area | Approximate Coverage | Key Characteristics | |-----------------------------|----------------------|---------------------| | Overall Statements / Lines | ~55% | Primarily happy-path unit tests | | Branch Coverage | ~40% | Limited exploration of conditional paths | | Dashboard Routes | ~42% lines / ~25% branches | Core data aggregation paths under-tested | | Ombi Integration | 0% | No dedicated integration tests | | Background Poller | 0% | No unit or integration coverage | | SSE Streaming Endpoint | Excluded | Intentionally omitted from test suite | | Rate Limiting | Mostly skipped | Tests bypass production rate limit logic | | Frontend UI Components | Low | Limited coverage of conditional rendering | --- ## Technical Analysis of Gaps ### 1. Absence of Ombi Integration Tests **Technical Requirement:** Ombi integration involves request enrichment (`decorateRequestsWithArrLinks`), webhook processing, and cross-service linking between Ombi and *arr applications. These flows contain multiple conditional branches based on media type, ID variants, and admin status. **Current Gap:** Zero dedicated integration tests exist for this subsystem despite its complexity and recent changes. **Technical Implication:** Without integration-level tests exercising the full request → enrichment → link generation pipeline, regression protection for this integration is absent. This reduces confidence in changes affecting ID extraction logic (`theTvDbId`, `tvDbId`, `theMovieDbId`, etc.) and admin link generation. **Recommendation:** Create a dedicated integration test suite (`tests/integration/ombi.test.js`) covering: - Request fetching and filtering - `decorateRequestsWithArrLinks` behavior across movie/TV cases - Webhook payload processing - Link generation for both movie and TV requests under admin and non-admin contexts --- ### 2. Untested Background Polling Engine **Technical Requirement:** The poller (`server/utils/poller.js`) is responsible for periodic data aggregation from multiple external services, managing concurrency via a `polling` guard, notifying SSE subscribers, and handling graceful shutdown. It forms the backbone of the real-time dashboard behavior. **Current Gap:** No unit or integration tests cover this component. **Technical Implication:** Key behaviors such as concurrent execution prevention, subscriber notification, error recovery paths, and interval-based scheduling remain unverified. This creates a blind spot in testing the system's core data freshness mechanism. **Recommendation:** Implement unit tests for `poller.js` focusing on: - Polling guard logic (`polling` flag behavior) - Subscriber registration and notification (`onPollComplete` / `offPollComplete`) - Error handling and recovery within `pollAllServices` - Graceful shutdown sequence - Behavior under varying `POLL_INTERVAL` configurations --- ### 3. Exclusion of SSE Endpoint from Testing **Technical Requirement:** The `/api/dashboard/stream` endpoint implements Server-Sent Events for real-time push updates. It is central to the application's stated real-time capabilities and interacts directly with the poller. **Current Gap:** This endpoint is intentionally excluded from the test suite. **Technical Implication:** There is no automated verification of event emission timing, client connection lifecycle, proper cleanup on disconnect, or correct payload formatting for different event types (downloads, history, requests). **Recommendation:** Add integration tests for the SSE endpoint that verify: - Successful connection and initial data push - Event emission on poll completion - Proper handling of multiple concurrent clients - Connection cleanup and resource release --- ### 4. Limited Coverage of Rate Limiting and Error Scenarios **Technical Requirement:** Production rate limiting and external service error handling (timeouts, 5xx responses, rate limit responses) are critical for system stability when interacting with Sonarr, Radarr, Ombi, and download clients. **Current Gap:** Most integration tests bypass rate limiting via `SKIP_RATE_LIMIT`. There is minimal coverage of failure modes using `nock`. **Technical Implication:** The test suite does not exercise the system's resilience characteristics under load or external service degradation. **Recommendation:** - Remove or minimize use of `SKIP_RATE_LIMIT` in integration tests - Add dedicated test cases using `nock` for: - Rate limit responses (429) - Server errors (5xx) - Timeouts - Partial failures across multiple services --- ### 5. Insufficient Frontend Component Coverage **Technical Requirement:** The frontend renders dynamic UI elements based on data properties such as `arrType`, `arrLink`, `isAdmin`, `movieName`, and `seriesName`. Conditional rendering logic directly affects user-facing features (e.g., service icons and deep links). **Current Gap:** Coverage of `client/src/ui/downloads.js` and related components is low, with limited testing of conditional rendering paths. **Technical Implication:** Changes to rendering logic for admin features or media-type differentiation carry higher regression risk without targeted component tests. **Recommendation:** Add Vitest + jsdom tests for: - `createServiceIcons` function and its conditional logic - Rendering of Radarr/Sonarr icons based on `arrType` and `arrLink` - Behavior under admin vs non-admin states - Differentiation between movie (`movieName`) and series (`seriesName`) content --- ## Recommended Coverage Targets Based on the architectural characteristics of the system (real-time event-driven flows, multiple external integrations, admin-only features), the following targets are recommended: | Metric | Current | Recommended Minimum | Stretch Target | |--------------------|---------|---------------------|----------------| | Statements / Lines | ~55% | 75% | 85% | | Branches | ~40% | 65% | 75% | | Functions | - | 70% | 80% | These targets should be enforced via CI and reviewed periodically as new features are added. --- ## Prioritized Action Plan ### Phase 1 (Immediate) 1. Create Ombi integration test suite 2. Add unit tests for the poller component 3. Implement basic SSE endpoint integration tests ### Phase 2 (Short Term) 4. Expand rate limiting and error scenario coverage 5. Increase frontend component test coverage for downloads UI 6. Raise CI coverage thresholds to recommended minimums ### Phase 3 (Ongoing) 7. Maintain test coverage during feature development 8. Add contract-style tests for external service interactions (future consideration) 9. Review and update test strategy quarterly as the system evolves --- ## Conclusion Effective test coverage for this system requires deliberate focus on the real-time, integration-heavy, and conditionally-rendered aspects of the architecture. The recommendations above are designed to align test scope with the technical requirements of background processing, external service resilience, and dynamic UI behavior. Implementation of these improvements will provide stronger regression protection and greater confidence in the correctness of core system behaviors.
Gandalf added the Kind/Testing label 2026-05-27 23:43:42 +01:00
Gandalf commented 2026-05-27 23:59:27 +01:00
Author
Owner
Copy Link
Copy Source

Implementation plan:

Implementation Plan - Issue #60 Testing Scope Review

This plan addresses the test coverage recommendations outlined in Issue #60. After performing a detailed analysis of the repository's existing test suites, we have discovered that several of the gaps claimed in the issue are already addressed in the codebase, while others are highly valid.

This plan presents our technical analysis, clarifies the discrepancies, and outlines a precise roadmap to implement the actual missing coverage.

Technical Analysis of Gaps

We compared the claims made in Issue #60 with the current state of the codebase. Here is our assessment:

1. Absence of Ombi Integration Tests

  • Issue #60 Claim: "Zero dedicated integration tests exist for this subsystem despite its complexity and recent changes."
  • Actual State: A comprehensive integration test suite exists at ombi.test.js (1,102 lines). It fully tests user request filtering, admin showAll params, TV requests decoration with tvDbId (Issue #58), webhook status/enable/test routes, API errors, and local loopback fallbacks.
  • Our Assessment: Outdated/Disagree. This gap is already fully addressed. No immediate action is required here.

2. Untested Background Polling Engine

  • Issue #60 Claim: "No unit or integration tests cover this component [server/utils/poller.js]."
  • Actual State: Verified. There are indeed zero tests covering poller.js.
  • Our Assessment: Fully Agree. This is a critical gap. The background poller coordinates service initialization, timed cache refreshes, active client stream callbacks, and intelligent webhook-based polling bypasses.
  • Plan: Create a dedicated unit test suite at tests/unit/utils/poller.test.js to cover its core behaviors.

3. Exclusion of SSE Endpoint from Testing

  • Issue #60 Claim: "This endpoint [/api/dashboard/stream] is intentionally excluded from the test suite."
  • Actual State: Basic integration tests for /api/dashboard/stream actually exist at the bottom of dashboard.test.js. They verify that the stream properly initializes, outputs correctly formatted SSE data, and honors the showAll admin parameter (utilizing a testClose=true parameter to terminate the connection gracefully after the initial payload).
  • Our Assessment: Partially Outdated. While basic stream payload tests exist, we can expand coverage to test stream lifecycle events, heartbeat transmissions, and multi-subscriber connection cleanup.
  • Plan: Add advanced stream tests to tests/integration/dashboard.test.js exercising connection termination cleanup (req.on('close')) to verify it successfully deregisters callbacks and prevents memory leaks.

4. Limited Coverage of Rate Limiting and Error Scenarios

  • Issue #60 Claim: "Most integration tests bypass rate limiting via SKIP_RATE_LIMIT. There is minimal coverage of failure modes using nock."
  • Actual State: Standard integration tests set SKIP_RATE_LIMIT = '1' globally in setup.js to prevent test runners from exhausting the API limits from a single localhost origin.
  • Our Assessment: Partially Valid (Requires Nuance). Bypassing rate limiting generally is required for test suite stability. However, we have zero automated validation that rate limiting actually functions in production.
  • Plan: Create an isolated integration test file tests/integration/rateLimiter.test.js that explicitly clears/overrides process.env.SKIP_RATE_LIMIT and makes sequential requests to verify that the rate limiter triggers (returns 429) as expected.

5. Insufficient Frontend Component Coverage

  • Issue #60 Claim: "Coverage of client/src/ui/downloads.js and related components is low, with limited testing of conditional rendering paths."
  • Actual State: Verified. downloads.test.js only tests renderTagBadges(). Key DOM construction functions like createServiceIcons() (conditionally rendering Sonarr/Radarr/Ombi deep-links based on arrType, arrLink, and state.isAdmin) are untested.
  • Our Assessment: Fully Agree.
  • Plan: Expand frontend component coverage in tests/frontend/ui/downloads.test.js to test createServiceIcons() and createClientLogo() across all user roles and media types.

User Review Required

Note

All changes are non-breaking and solely expand test coverage.

We recommend retaining the global SKIP_RATE_LIMIT settings in tests/setup.js for overall suite stability, while utilizing a dedicated test file to isolate and assert rate-limiting logic.

Open Questions

Important

Please review and provide your feedback on the following questions:

  1. Rate Limiting Tests: Do you agree with creating a dedicated tests/integration/rateLimiter.test.js file to test rate limits in isolation, rather than globally re-enabling it (which would break other integration tests)?
  2. Poller Timer Testing: For the background poller setInterval testing, do you prefer using Vitest's fake timers (vi.useFakeTimers()) to speed up clock ticks, or asserting on the state machine functions (pollAllServices(), etc.) directly? (We recommend fake timers for comprehensive coverage).

Proposed Changes

Tests Component

[NEW] poller.test.js

Create a new unit test suite testing:

  • Concurrency prevention: verify that pollAllServices() aborts early if polling = true.
  • Subscriber registry: verify that onPollComplete(cb) adds callbacks to the list, offPollComplete(cb) deletes them, and subscribers are successfully executed upon completing a poll.
  • Error recovery: verify that if a service poll throws an error, the polling state is reset to false in the finally block so future polls are not permanently blocked.
  • Webhook bypasses: verify that shouldSkipInstancePolling() detects recent webhook activity in cache and correctly skips polling.
  • Global webhook fallback: verify that if no webhooks are seen globally within WEBHOOK_FALLBACK_TIMEOUT, a full poll is forced.

[NEW] rateLimiter.test.js

Create a dedicated rate limiter integration test suite:

  • Temporarily unset process.env.SKIP_RATE_LIMIT inside the test context.
  • Create a lightweight app instance with standard rate limit configurations.
  • Fire rapid requests using supertest to verify a 429 Too Many Requests is returned.
  • Restore original env vars after execution to ensure no side-effects leak.

[MODIFY] downloads.test.js

Modify and append new tests to cover:

  • createServiceIcons():
    • Renders an Ombi icon link when ombiLink is provided (for all users).
    • Renders Sonarr icon link for administrators when arrType === 'sonarr' and arrLink exists.
    • Renders Radarr icon link for administrators when arrType === 'radarr' and arrLink exists.
    • Renders nothing when no links are present or if user is non-admin.
  • createClientLogo():
    • Renders client logo <img> matching download.client and sets correct classes and titles.
    • Standard fallback: sets character avatar text when an image loading error occurs.

[MODIFY] dashboard.test.js

Append tests to cover SSE Stream Lifecycle details:

  • Connection heartbeat timing and event formatting.
  • Deregistering of onPollComplete and onHistoryUpdate listeners on client close/abort, preventing connection-based memory leaks.

Verification Plan

Automated Tests

  • Run the entire test suite locally to verify everything passes: 
    npm test
  • Run the coverage report to verify that statements, branches, and lines meet or exceed the coverage targets: 
    npm run test:coverage
Implementation plan: # Implementation Plan - Issue #60 Testing Scope Review This plan addresses the test coverage recommendations outlined in [Issue #60](file:///home/gordon/.gemini/antigravity-ide/brain/e4c5d5db-7e90-4905-a596-37bf27cb6766/issue_60.txt). After performing a detailed analysis of the repository's existing test suites, we have discovered that several of the gaps claimed in the issue are **already addressed** in the codebase, while others are highly valid. This plan presents our technical analysis, clarifies the discrepancies, and outlines a precise roadmap to implement the actual missing coverage. --- ## Technical Analysis of Gaps We compared the claims made in Issue #60 with the current state of the codebase. Here is our assessment: ### 1. Absence of Ombi Integration Tests * **Issue #60 Claim:** *"Zero dedicated integration tests exist for this subsystem despite its complexity and recent changes."* * **Actual State:** A comprehensive integration test suite exists at [ombi.test.js](file:///home/gordon/CascadeProjects/sofarr/tests/integration/ombi.test.js) (1,102 lines). It fully tests user request filtering, admin `showAll` params, TV requests decoration with `tvDbId` (Issue #58), webhook status/enable/test routes, API errors, and local loopback fallbacks. * **Our Assessment:** **Outdated/Disagree.** This gap is already fully addressed. No immediate action is required here. ### 2. Untested Background Polling Engine * **Issue #60 Claim:** *"No unit or integration tests cover this component [server/utils/poller.js]."* * **Actual State:** Verified. There are indeed zero tests covering [poller.js](file:///home/gordon/CascadeProjects/sofarr/server/utils/poller.js). * **Our Assessment:** **Fully Agree.** This is a critical gap. The background poller coordinates service initialization, timed cache refreshes, active client stream callbacks, and intelligent webhook-based polling bypasses. * **Plan:** Create a dedicated unit test suite at `tests/unit/utils/poller.test.js` to cover its core behaviors. ### 3. Exclusion of SSE Endpoint from Testing * **Issue #60 Claim:** *"This endpoint [/api/dashboard/stream] is intentionally excluded from the test suite."* * **Actual State:** Basic integration tests for `/api/dashboard/stream` actually exist at the bottom of [dashboard.test.js](file:///home/gordon/CascadeProjects/sofarr/tests/integration/dashboard.test.js#L1037-L1139). They verify that the stream properly initializes, outputs correctly formatted SSE data, and honors the `showAll` admin parameter (utilizing a `testClose=true` parameter to terminate the connection gracefully after the initial payload). * **Our Assessment:** **Partially Outdated.** While basic stream payload tests exist, we can expand coverage to test stream lifecycle events, heartbeat transmissions, and multi-subscriber connection cleanup. * **Plan:** Add advanced stream tests to `tests/integration/dashboard.test.js` exercising connection termination cleanup (`req.on('close')`) to verify it successfully deregisters callbacks and prevents memory leaks. ### 4. Limited Coverage of Rate Limiting and Error Scenarios * **Issue #60 Claim:** *"Most integration tests bypass rate limiting via SKIP_RATE_LIMIT. There is minimal coverage of failure modes using nock."* * **Actual State:** Standard integration tests set `SKIP_RATE_LIMIT = '1'` globally in [setup.js](file:///home/gordon/CascadeProjects/sofarr/tests/setup.js#L15) to prevent test runners from exhausting the API limits from a single localhost origin. * **Our Assessment:** **Partially Valid (Requires Nuance).** Bypassing rate limiting generally is required for test suite stability. However, we have zero automated validation that rate limiting actually functions in production. * **Plan:** Create an isolated integration test file `tests/integration/rateLimiter.test.js` that explicitly clears/overrides `process.env.SKIP_RATE_LIMIT` and makes sequential requests to verify that the rate limiter triggers (returns 429) as expected. ### 5. Insufficient Frontend Component Coverage * **Issue #60 Claim:** *"Coverage of client/src/ui/downloads.js and related components is low, with limited testing of conditional rendering paths."* * **Actual State:** Verified. [downloads.test.js](file:///home/gordon/CascadeProjects/sofarr/tests/frontend/ui/downloads.test.js) only tests `renderTagBadges()`. Key DOM construction functions like `createServiceIcons()` (conditionally rendering Sonarr/Radarr/Ombi deep-links based on `arrType`, `arrLink`, and `state.isAdmin`) are untested. * **Our Assessment:** **Fully Agree.** * **Plan:** Expand frontend component coverage in `tests/frontend/ui/downloads.test.js` to test `createServiceIcons()` and `createClientLogo()` across all user roles and media types. --- ## User Review Required > [!NOTE] > All changes are non-breaking and solely expand test coverage. We recommend **retaining** the global `SKIP_RATE_LIMIT` settings in `tests/setup.js` for overall suite stability, while utilizing a dedicated test file to isolate and assert rate-limiting logic. --- ## Open Questions > [!IMPORTANT] > Please review and provide your feedback on the following questions: > > 1. **Rate Limiting Tests:** Do you agree with creating a dedicated `tests/integration/rateLimiter.test.js` file to test rate limits in isolation, rather than globally re-enabling it (which would break other integration tests)? > 2. **Poller Timer Testing:** For the background poller `setInterval` testing, do you prefer using Vitest's fake timers (`vi.useFakeTimers()`) to speed up clock ticks, or asserting on the state machine functions (`pollAllServices()`, etc.) directly? (We recommend fake timers for comprehensive coverage). --- ## Proposed Changes ### Tests Component #### [NEW] [poller.test.js](file:///home/gordon/CascadeProjects/sofarr/tests/unit/utils/poller.test.js) Create a new unit test suite testing: * Concurrency prevention: verify that `pollAllServices()` aborts early if `polling = true`. * Subscriber registry: verify that `onPollComplete(cb)` adds callbacks to the list, `offPollComplete(cb)` deletes them, and subscribers are successfully executed upon completing a poll. * Error recovery: verify that if a service poll throws an error, the `polling` state is reset to `false` in the `finally` block so future polls are not permanently blocked. * Webhook bypasses: verify that `shouldSkipInstancePolling()` detects recent webhook activity in `cache` and correctly skips polling. * Global webhook fallback: verify that if no webhooks are seen globally within `WEBHOOK_FALLBACK_TIMEOUT`, a full poll is forced. #### [NEW] [rateLimiter.test.js](file:///home/gordon/CascadeProjects/sofarr/tests/integration/rateLimiter.test.js) Create a dedicated rate limiter integration test suite: * Temporarily unset `process.env.SKIP_RATE_LIMIT` inside the test context. * Create a lightweight app instance with standard rate limit configurations. * Fire rapid requests using `supertest` to verify a `429 Too Many Requests` is returned. * Restore original env vars after execution to ensure no side-effects leak. #### [MODIFY] [downloads.test.js](file:///home/gordon/CascadeProjects/sofarr/tests/frontend/ui/downloads.test.js) Modify and append new tests to cover: * `createServiceIcons()`: * Renders an Ombi icon link when `ombiLink` is provided (for all users). * Renders Sonarr icon link for administrators when `arrType === 'sonarr'` and `arrLink` exists. * Renders Radarr icon link for administrators when `arrType === 'radarr'` and `arrLink` exists. * Renders nothing when no links are present or if user is non-admin. * `createClientLogo()`: * Renders client logo `<img>` matching `download.client` and sets correct classes and titles. * Standard fallback: sets character avatar text when an image loading error occurs. #### [MODIFY] [dashboard.test.js](file:///home/gordon/CascadeProjects/sofarr/tests/integration/dashboard.test.js) Append tests to cover SSE Stream Lifecycle details: * Connection heartbeat timing and event formatting. * Deregistering of `onPollComplete` and `onHistoryUpdate` listeners on client close/abort, preventing connection-based memory leaks. --- ## Verification Plan ### Automated Tests * Run the entire test suite locally to verify everything passes: ```bash npm test ``` * Run the coverage report to verify that statements, branches, and lines meet or exceed the coverage targets: ```bash npm run test:coverage ```
Gandalf commented 2026-05-27 23:59:59 +01:00
Author
Owner
Copy Link
Copy Source

Feedback on Implementation Plan (Issue #60)

Overall Assessment

The plan is well-structured, pragmatic, and correctly identifies which gaps from Issue #60 are already addressed versus which ones require action. The proposed test additions target the right architectural areas (real-time polling, SSE lifecycle, rate limiting resilience, and admin UI rendering). I support moving forward with this plan.

Detailed Feedback by Section

1. Ombi Integration Tests
Correct assessment. The existing ombi.test.js (1,102 lines) already provides strong coverage, including recent tvDbId handling. No action required.

2. Background Poller (poller.test.js)
Excellent scope. The planned coverage areas (concurrency guard, subscriber registry, error recovery, webhook bypass logic, and fallback timeout) are the highest-value behaviors to test.

Recommendation on Testing Approach:
Use a hybrid strategy:

  • vi.useFakeTimers() for timing-dependent tests (interval behavior and WEBHOOK_FALLBACK_TIMEOUT).
  • Direct function testing for pure logic (shouldSkipInstancePolling(), error recovery in finally, subscriber management).

This combination provides both behavioral and timing coverage without introducing flakiness.

3. SSE Endpoint Tests
Good direction. Adding tests for connection lifecycle (req.on('close')), callback deregistration, and memory leak prevention is valuable.
Additional suggestion: Consider also verifying that different event types (downloads, history, requests) are emitted with correct formatting.

4. Rate Limiting Tests (rateLimiter.test.js)
Strong approach. Creating an isolated test file that temporarily unsets SKIP_RATE_LIMIT is the correct technical decision. This maintains overall suite stability while still validating production rate limiting behavior.

5. Frontend Component Tests (downloads.test.js)
Very good. Expanding coverage of createServiceIcons() across all combinations of arrType, arrLink, and admin state will help prevent regressions on the recent link button issues. Testing createClientLogo() fallback behavior is also useful.

Answers to Open Questions

1. Rate Limiting Tests
Yes — I strongly recommend keeping the dedicated rateLimiter.test.js approach. Isolating the test is architecturally cleaner than globally re-enabling rate limiting.

2. Poller Timer Testing
I recommend using fake timers (vi.useFakeTimers()) for comprehensive coverage of time-based behavior, combined with direct state machine testing for non-timing logic.

Additional Recommendations

  • If we later implement the active downloads arrLink decoration (from the related Radarr link issue), consider adding a small test for decorateDownloadsWithArrLinks() to maintain consistency with the existing Ombi decoration tests.
  • Consider adding a lightweight snapshot or contract test for SSE event payload structure to protect against accidental format changes.
  • In the poller tests, add one edge-case test for very low POLL_INTERVAL values.

Final Verdict

This is a solid, focused plan. It addresses the real gaps efficiently without over-engineering. I recommend proceeding with implementation, incorporating the hybrid timer approach for the poller and the isolated rate limiter test.

## Feedback on Implementation Plan (Issue #60) ### Overall Assessment The plan is well-structured, pragmatic, and correctly identifies which gaps from Issue #60 are already addressed versus which ones require action. The proposed test additions target the right architectural areas (real-time polling, SSE lifecycle, rate limiting resilience, and admin UI rendering). I support moving forward with this plan. ### Detailed Feedback by Section **1. Ombi Integration Tests** Correct assessment. The existing `ombi.test.js` (1,102 lines) already provides strong coverage, including recent `tvDbId` handling. No action required. **2. Background Poller (`poller.test.js`)** Excellent scope. The planned coverage areas (concurrency guard, subscriber registry, error recovery, webhook bypass logic, and fallback timeout) are the highest-value behaviors to test. **Recommendation on Testing Approach**: Use a **hybrid strategy**: - `vi.useFakeTimers()` for timing-dependent tests (interval behavior and `WEBHOOK_FALLBACK_TIMEOUT`). - Direct function testing for pure logic (`shouldSkipInstancePolling()`, error recovery in `finally`, subscriber management). This combination provides both behavioral and timing coverage without introducing flakiness. **3. SSE Endpoint Tests** Good direction. Adding tests for connection lifecycle (`req.on('close')`), callback deregistration, and memory leak prevention is valuable. **Additional suggestion**: Consider also verifying that different event types (downloads, history, requests) are emitted with correct formatting. **4. Rate Limiting Tests (`rateLimiter.test.js`)** Strong approach. Creating an isolated test file that temporarily unsets `SKIP_RATE_LIMIT` is the correct technical decision. This maintains overall suite stability while still validating production rate limiting behavior. **5. Frontend Component Tests (`downloads.test.js`)** Very good. Expanding coverage of `createServiceIcons()` across all combinations of `arrType`, `arrLink`, and admin state will help prevent regressions on the recent link button issues. Testing `createClientLogo()` fallback behavior is also useful. ### Answers to Open Questions **1. Rate Limiting Tests** Yes — I strongly recommend keeping the dedicated `rateLimiter.test.js` approach. Isolating the test is architecturally cleaner than globally re-enabling rate limiting. **2. Poller Timer Testing** I recommend using **fake timers (`vi.useFakeTimers()`)** for comprehensive coverage of time-based behavior, combined with direct state machine testing for non-timing logic. ### Additional Recommendations - If we later implement the active downloads `arrLink` decoration (from the related Radarr link issue), consider adding a small test for `decorateDownloadsWithArrLinks()` to maintain consistency with the existing Ombi decoration tests. - Consider adding a lightweight snapshot or contract test for SSE event payload structure to protect against accidental format changes. - In the poller tests, add one edge-case test for very low `POLL_INTERVAL` values. ### Final Verdict This is a solid, focused plan. It addresses the real gaps efficiently without over-engineering. I recommend proceeding with implementation, incorporating the hybrid timer approach for the poller and the isolated rate limiter test.
Gandalf commented 2026-05-28 01:39:48 +01:00
Author
Owner
Copy Link
Copy Source

Resolved in commit aec0447. This has been merged into main (merge commit 52806d0) and officially tagged in point release v1.7.30.

Resolved in commit aec0447. This has been merged into main (merge commit 52806d0) and officially tagged in point release v1.7.30.
Gandalf commented 2026-05-28 07:03:33 +01:00
Author
Owner
Copy Link
Copy Source

Documentation formatting fix pushed to develop (a4af160), main (4107bdf), and release/1.7.30 (585092a). Tag v1.7.30 recreated on main.

Documentation formatting fix pushed to develop (a4af160), main (4107bdf), and release/1.7.30 (585092a). Tag v1.7.30 recreated on main.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Docker
Docker image and container packaging
 Area/Download Clients
Download client integrations
 Area/Frontend
Client-side UI and build tooling
 Area/History
History and completed downloads views
 Area/Logging
Log streaming, file handling, and debug infrastructure
 Area/Matching
Matching and correlation logic
 Area/Proxy
Upstream service proxy routes
 Area/SSE
Server-Sent Events and real-time streaming
 Area/Webhooks
Webhook processing and reliability
 Compat/Breaking
Breaking change that won't be backward compatible
 Compat/Non-Breaking
Non-breaking compatibility change
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
No Label Kind/Testing
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Gandalf
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Gandalf/sofarr#60
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?