Graylog2 / Graylog2/graylog2-server
Collectors frontend tests: mock hooks per file and stop stubbing sibling components
- Dominant language
- Java
- Stars
- 8.1k
- Forks
- 1.1k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 217
Description
## Motivation
Review of #27252 (https://github.com/Graylog2/graylog2-server/pull/27252#discussion_r3935079085 and #discussion_r3934950686) flagged two patterns in the collectors test suite that hide runtime errors and make tests brittle:
1. Mocking the hooks index (`components/collectors/hooks`) instead of individual hook files.
2. Replacing collectors-owned components with stubs, so an error inside the real component is never caught.
Both patterns are module-wide, not specific to that PR, so they are addressed here in one pass. Rules: `graylog2-web-interface/CONTRIBUTING.md`, sections "Mocking" and "Assertions and Queries".
## Current state (46 test files, 126 `jest.mock` calls)
### Hook mocking, three coexisting styles
Whole-index automock, `jest.mock('../hooks')`, 5 files:
- components/collectors/settings/CollectorsSettings.test.tsx
- components/collectors/deployment/DeployTab.test.tsx
- components/collectors/overview/onboarding/IngestEndpointStrip.test.tsx
- components/collectors/overview/FirstOnboarding.test.tsx
- pages/CollectorsOverviewPage.test.tsx
Index factory mock (partial module, every unlisted hook becomes `undefined`), 11 files:
- components/collectors/fleets/FleetDetail.test.tsx
- components/collectors/fleets/CollectorsFleets.test.tsx
- components/collectors/instances/BulkActions.test.tsx
- components/collectors/instances/CollectorsInstances.test.tsx
- components/collectors/instances/InstanceDetailDrawer.test.tsx
- components/collectors/overview/RecentActivity.test.tsx
- components/collectors/overview/CollectorsOverview.test.tsx
- pages/CollectorsDeploymentPage.test.tsx
- pages/CollectorsFleetsPage.test.tsx
- pages/CollectorsInstancesPage.test.tsx
- pages/CollectorsFleetDetailPage.test.tsx
Per-hook-file mock (target style), roughly 20 files already do this, e.g. TokenStep, EnrollingHostsList, ReassignFleetModal, InstanceActions, EnrollmentTokenList, ConnectionSuccess.
### Stubs of collectors-owned components (12 stubs, 11 files)
| Test | Stubbed component(s) |
|---|---|
| components/collectors/overview/CollectorsOverview.test.tsx | RecentActivity, FirstOnboarding |
| components/collectors/overview/FirstOnboarding.test.tsx | WaitingForConnection |
| components/collectors/fleets/FleetDetail.test.tsx | InstanceDetailDrawer |
| components/collectors/instances/BulkActions.test.tsx | ReassignFleetModal |
| components/collectors/instances/InstanceActions.test.tsx | ReassignFleetModal |
| pages/CollectorsOnboardingInstancePage.test.tsx | ConnectionSuccess |
| pages/CollectorsDeploymentPage.test.tsx | DeployTab, EnrollmentTokenList |
| pages/CollectorsFleetsPage.test.tsx | CollectorsFleets |
| pages/CollectorsInstancesPage.test.tsx | CollectorsInstances |
| pages/CollectorsFleetDetailPage.test.tsx | FleetDetail |
| pages/CollectorsOverviewPage.test.tsx | CollectorsOverview |
### Stubs of shared components (out of scope unless agreed otherwise)
- `components/common/PaginatedEntityTable`: FleetDetail, CollectorsInstances, CollectorsFleets tests
- `components/common/CreateButton`: CollectorsFleetsPage test (precedent: PipelinesOverviewPage.test.tsx)
- `components/common/Tooltip` passthrough: DeployTab, InstallCommand, FirstOnboarding tests
## Proposed work
### Part 1: hook mocks (mechanical)
- Replace all 16 index mocks with per-file mocks, e.g. `jest.mock('components/collectors/hooks/useCollectorsConfig')`. The index re-exports resolve to the mocked module, so components importing from `../hooks` need no change.
- Where a factory currently lists hooks, mock each listed hook's file with automock and set return values via `asMock(...)` in `beforeEach`.
- Reuse `components/collectors/testing/{fixtures,mockMutations,mockPermissions}.ts` for return values.
### Part 2: component stubs (needs a rule first)
Proposed rule, to be confirmed in review:
- A component test renders its real children and mocks their hooks. No stubs of collectors-owned components.
- A page test that only asserts the permission/redirect gate may stub the page content, since the content has its on the page renders for real.
Under that rule:
- Remove: RecentActivity, FirstOnboarding (in CollectorsOverview.test), WaitingForConnection, InstanceDetailDrawer, ReassignFleetModal (x2), ConnectionSuccess stubs. Mock their hooks instead.
- Keep, with a comment stating the rule: page-content stubs in CollectorsDeploymentPage, CollectorsFleetsPage, CollectorsInstancesPage, CollectorsFleetDetailPage, CollectorsOverviewPage tests.
- Decide separately whether `PaginatedEntityTable` stubs stay; it is stubbed in other modules too.
## Acceptance
- No `jest.mock('<...>/hooks')` or `jest.mock('components/collectors/hooks', ...)` left in `src/components/collectors*`.
- Component tests stub no collectors-owned components.
- Remaining page-content stubs carry a one-line comment referencing this issue.
- `yarn test src/components/collectors src/pages/Collectors` green; `yarn tsc` and eslint clean.
## Notes
- Removing the strip stub in #27252 already exposed one wrong assertion the stub had hidden (configured cluster without input shows "No ingest input exists", not the confirm title). Expect more of these.
- Related: #27322 (strip not shown for a failed input) may add another consumer of IngestEndpointStrip
Contributor guide
Research direction
Start with graylog2-web-interface/CONTRIBUTING.md sections “Mocking” and “Assertions and Queries”, then review the listed collector tests and existing per-hook examples. Run the collector test command to identify failures while replacing index mocks and collectors-owned component stubs. Done means the stated mock patterns and stubs are removed or documented, and the collector tests, tsc, and eslint pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- frontend, testing
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100