Skip to content

tests: standardize MSW error simulation and reduce handler boilerplate#552

Open
yurii-vasyliev wants to merge 1 commit intomainfrom
improve-test-env
Open

tests: standardize MSW error simulation and reduce handler boilerplate#552
yurii-vasyliev wants to merge 1 commit intomainfrom
improve-test-env

Conversation

@yurii-vasyliev
Copy link
Copy Markdown
Collaborator

Summary

Standardize how MSW test handlers simulate endpoint errors and reduce
copy-pasted boilerplate across handler files.

Problem

The MSW handler layer had accumulated several maintenance issues:

  1. Two ad-hoc error patterns — some handlers threw new HttpResponse(null, { status: 500 }) (null body), others threw the shared ENDPOINT_STATUS_API_ERROR constant (JSON body). Neither pattern was documented or easily discoverable.
  2. Shared mutable response objectENDPOINT_STATUS_API_ERROR is a single HttpResponse instance reused across all handlers, which can cause subtle issues when MSW mutates response internals.
  3. Duplicated path-matching logic — every handler that respected targeted setEndpointStatus({ status, path }) reimplemented the same !endpointStatus.path || endpointStatus.path.includes(…) check.
  4. Dead / redundant code — a duplicate ChangeComputersAccessGroup handler, two overlapping catch-all handlers in browser.ts, an unreachable ?? [] fallback in constants.ts, and an empty endpointsToIntercept.json that was no longer imported.

Changes

New infrastructure (_constants.ts, _helpers.ts)

Export Purpose
createEndpointStatusError(status?) Fresh JSON-body 500 response ({ error, message }). Use where tests assert on the parsed error body.
createEndpointStatusNetworkError(status?) Fresh null-body 500 response. Use where tests only care about the HTTP status code.
shouldApplyEndpointStatus(path?) Returns true when the global endpoint-status controller has a non-default status that applies to the given handler path.

Handler migration (17 files)

Every handler that checked getEndpointStatus() for error simulation was
updated to use the new helpers. The original error body shape (null vs JSON)
was preserved per-handler to avoid changing test behavior.

Cleanup

  • accessGroup.ts — removed dead duplicate handler
  • browser.ts — merged two catch-all handlers; unmatched requests now warn + passthrough instead of returning 404
  • constants.ts — removed unreachable ?? []
  • endpointsToIntercept.json — deleted (empty, unreferenced)
  • Removed unused type imports (GetInstanceParams, GetInstancesParams, GetActivitiesParams, ApiPaginatedResponse) left over from strict generic annotations that were simplified

Testing

  • Full Vitest suite: 1600 passed, 0 new failures
  • ESLint: 0 errors
  • The 3 failures in the CI run are pre-existing flaky tests also present on main

Release Impact

According to the Landscape Server Release Cycle, this change will target the following release cycle:

  • Target Branch: dev / main (Beta)
  • Version Impact:
    • Patch (Fix)
    • Minor (Feature)
    • Major (Breaking)

Checklist

  • Changeset Added: I have run pnpm changeset and committed the resulting .md file.
  • UI Verified: I have verified the changes locally.
  • Linting: No linting errors are present (especially in scripts/).

Versioning Reminder

Important

This repository now uses CalVer ($YY.0M.Point.Patch$).
Please ensure your changeset description is clear, as it will be automatically added to the CHANGELOG.md upon merging to main.

…boilerplate

- Add `createEndpointStatusError()` and `createEndpointStatusNetworkError()`
  factory functions to replace the shared static `ENDPOINT_STATUS_API_ERROR`
  constant and scattered inline `new HttpResponse(null, { status: 500 })`
- Add `shouldApplyEndpointStatus(path)` helper to replace duplicated
  path-matching logic across 17 handler files
- Remove dead duplicate `ChangeComputersAccessGroup` handler in accessGroup.ts
- Merge redundant catch-all handlers in browser.ts; unmatched requests now
  log a warning and passthrough instead of returning 404
- Remove unreachable `?? []` fallback in MSW_ENDPOINTS_TO_INTERCEPT
- Delete unused `endpointsToIntercept.json`
- Clean up unused type imports from migrated handlers
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the MSW test handler layer to standardize endpoint error simulation (JSON-body vs null-body) and reduce duplicated boilerplate around applying setEndpointStatus({ status, path }).

Changes:

  • Introduces shared helpers for endpoint-status applicability (shouldApplyEndpointStatus) and for generating fresh MSW error responses (createEndpointStatusError, createEndpointStatusNetworkError).
  • Migrates many handler files to use the shared helpers, removing repeated path-matching and avoiding reuse of a shared HttpResponse instance.
  • Cleans up redundant/dead test infrastructure (duplicate handler removal, catch-all handler behavior, unreachable fallback, unused/deleted intercept list).

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/tests/server/handlers/_constants.ts Adds factory helpers for fresh JSON/null-body error responses; deprecates shared constant usage.
src/tests/server/handlers/_helpers.ts Adds shouldApplyEndpointStatus and uses it to simplify list endpoint generation.
src/tests/server/handlers/activity.ts Migrates endpoint-status error simulation to new helpers (introduces a regression—see comments).
src/tests/server/handlers/aptSource.ts Migrates endpoint-status error simulation for APT source deletion.
src/tests/server/handlers/accessGroup.ts Removes a duplicate/dead handler.
src/tests/server/handlers/distributions.ts Switches to createEndpointStatusNetworkError for error simulation.
src/tests/server/handlers/eventsLog.ts Uses shouldApplyEndpointStatus + network-error factory for scoped error simulation.
src/tests/server/handlers/features.ts Uses createEndpointStatusNetworkError for standardized error simulation.
src/tests/server/handlers/instance.ts Updates computers-related handlers to use shared endpoint-status helpers.
src/tests/server/handlers/organisationPreferences.ts Adds scoped endpoint-status error simulation using shared helpers.
src/tests/server/handlers/packageProfile.ts Replaces shared error constant with per-invocation error factory + shared applicability helper.
src/tests/server/handlers/packages.ts Uses shared applicability helper and network-error factory for errors.
src/tests/server/handlers/process.ts Uses shared applicability helper and network-error factory for errors.
src/tests/server/handlers/rebootProfiles.ts Uses shared applicability helper and error factories for list/delete endpoints.
src/tests/server/handlers/removalProfiles.ts Uses shared applicability helper and network-error factory for errors/empty.
src/tests/server/handlers/repositoryProfiles.ts Uses shared applicability helper and network-error factory for errors.
src/tests/server/handlers/script.ts Uses shared applicability helper and network-error factory for errors/empty.
src/tests/server/handlers/scriptProfiles.ts Uses shared applicability helper and error factories across list/detail/activities endpoints.
src/tests/server/handlers/snap.ts Uses shared applicability helper and network-error factory for installed snaps errors.
src/tests/server/handlers/ubuntuPro.ts Uses shared applicability helper and JSON-error factory for token attach/detach errors.
src/tests/server/handlers/wslProfiles.ts Uses shared applicability helper and network-error factory for errors/empty.
src/tests/endpointsToIntercept.json Deletes an unused empty intercept list file.
src/tests/browser.ts Simplifies catch-all handling; unmatched requests now warn + passthrough.
src/constants.ts Removes unreachable ?? [] from MSW intercept env parsing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if (endpointStatus.status === "error") {
throw new HttpResponse(null, { status: 500 });
if (shouldApplyEndpointStatus("activities/:id")) {
throw createEndpointStatusNetworkError();
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldApplyEndpointStatus("activities/:id") becomes true for any non-default status (including empty), but this handler now throws a 500 unconditionally when it matches. Previously only status === "error" triggered the 500. Please read status from getEndpointStatus() and only throw createEndpointStatusNetworkError() when status === "error" (and optionally handle empty explicitly if desired).

Suggested change
throw createEndpointStatusNetworkError();
const { status } = getEndpointStatus();
if (status === "error") {
throw createEndpointStatusNetworkError();
}

Copilot uses AI. Check for mistakes.

if (endpointStatus.status === "error") {
throw ENDPOINT_STATUS_API_ERROR;
if (shouldApplyEndpointStatus("activities/revert")) {
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the activities/revert handler, shouldApplyEndpointStatus("activities/revert") is used as the sole condition to throw an error response. Since shouldApplyEndpointStatus returns true for both status: "error" and status: "empty", this will incorrectly throw a 500 for the empty case. Please gate the throw on getEndpointStatus().status === "error" (and keep empty behavior non-500).

Suggested change
if (shouldApplyEndpointStatus("activities/revert")) {
if (
shouldApplyEndpointStatus("activities/revert") &&
getEndpointStatus().status === "error"
) {

Copilot uses AI. Check for mistakes.

if (endpointStatus.status === "error") {
throw ENDPOINT_STATUS_API_ERROR;
if (shouldApplyEndpointStatus("activities/reapply")) {
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the activities/reapply handler, shouldApplyEndpointStatus("activities/reapply") currently triggers a 500 for any non-default status (including empty). This changes behavior from only erroring on status === "error". Please fetch the endpoint status and only throw createEndpointStatusError() when status === "error".

Suggested change
if (shouldApplyEndpointStatus("activities/reapply")) {
const status = getEndpointStatus("activities/reapply");
if (status === "error") {

Copilot uses AI. Check for mistakes.
Comment on lines 21 to 26
http.delete(`${API_URL}repository/apt-source/:sourceId`, () => {
const endpointStatus = getEndpointStatus();

if (
!endpointStatus.path ||
(endpointStatus.path &&
endpointStatus.path === "repository/apt-source/:sourceId")
) {
if (endpointStatus.status === "error") {
throw HttpResponse.json(ENDPOINT_STATUS_API_ERROR, { status: 500 });
if (shouldApplyEndpointStatus("repository/apt-source/:sourceId")) {
const { status } = getEndpointStatus();
if (status === "error") {
throw createEndpointStatusNetworkError();
}
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint previously attempted to return a JSON error response (via HttpResponse.json(...)) when endpoint-status was set to error. Switching to createEndpointStatusNetworkError() changes the error body to null, which can affect client error parsing and make this handler inconsistent with other mutation handlers that use createEndpointStatusError(). Consider using createEndpointStatusError() here if tests/UI expect the standard { error, message } body.

Copilot uses AI. Check for mistakes.
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.

2 participants