fix(detectors): Only render service incidents overlay on cron monitors #106308
Conversation
github-actions
bot
added
the
Scope: Frontend
jaydgoss
force-pushed
the
jaygoss/new-688-sentry-outage-warning-block-incorrectly-shown-on-uptime
branch
from
cf0adb1 to
86ea408
Compare
malwilley
left a comment
malwilley
left a comment
There was a problem hiding this comment.
Looks good! Just a couple small comments
| import CronDetectorsList from 'sentry/views/detectors/list/cron'; | ||
|
|
||
| // Mock the service incidents component to verify it's rendered | ||
| jest.mock('sentry/views/insights/crons/components/serviceIncidents', () => ({ |
There was a problem hiding this comment.
Can we check for the component's existence without mocking the component? It makes the test very brittle, it would be better if we could test for something in the actual component
| // Name | ||
| expect(within(row).getByText('Uptime Detector')).toBeInTheDocument(); | ||
|
|
||
| // Should NOT render service incidents overlay |
There was a problem hiding this comment.
nit: generally we don't test for the absence of elements, it's not very useful and will pretty much always pass
| showCursor | ||
| cursorOffsets={{right: 40}} | ||
| additionalUi={<CronServiceIncidents timeWindowConfig={timeWindowConfig} />} | ||
| additionalUi={renderTimelineOverlay?.({timeWindowConfig})} |
There was a problem hiding this comment.
This change makes sense! Certainly better than it being hardcoded with CronServiceIncidents
| incident_updates: [], | ||
| }); | ||
|
|
||
| fetchMock.mockResponse(req => |
There was a problem hiding this comment.
@jaydgoss we use MockApiClient to mock http requests: https://develop.sentry.dev/frontend/network-requests/#testing-components--hooks-with-network-requests
There was a problem hiding this comment.
Will investigate further but it seems the existing mocks for service incidents are not using MockApiClient, but fetchMock instead https://github.com/getsentry/sentry/blob/37a6465f1f80a07258a97d493dc726f67123d7c6/static/app/views/nav/primary/serviceIncidents.spec.tsx
There was a problem hiding this comment.
Oh, sorry, it does look like this component is doing something unique! Carry on, looks like this is the best way to mock it
jaydgoss
force-pushed
the
jaygoss/new-688-sentry-outage-warning-block-incorrectly-shown-on-uptime
branch
from
9a40d86 to
8ca82dc
Compare
jaydgoss
force-pushed
the
jaygoss/new-688-sentry-outage-warning-block-incorrectly-shown-on-uptime
branch
3 times, most recently
from
f7840f8 to
0d83685
Compare
The CronServiceIncidents overlay was being rendered on all detector types including uptime monitors. This moves the overlay rendering to a context prop (renderTimelineOverlay) that is only provided by cron detectors, ensuring it doesn't appear on uptime monitors.
- Refactor `cron.spec.tsx` to verify component existence without mocking the component itself, by mocking `useServiceIncidents`. - Remove redundant negative test in `uptime.spec.tsx`. - Add `data-test-id` to `CronServiceIncidents` component for better testing.
- Replace `useServiceIncidents` hook mock with `fetchMock` to comply with frontend testing guidelines. - Use `ServiceIncidentFixture` and `StatusPageComponent` for robust test data. - Update test description to reflect service incident verification.
- Align `cron.spec.tsx` with `serviceIncidents.spec.tsx` by moving `fetchMock.resetMocks()` to `afterEach`.
jaydgoss
force-pushed
the
jaygoss/new-688-sentry-outage-warning-block-incorrectly-shown-on-uptime
branch
from
0d83685 to
e2fcabf
Compare
jaydgoss
deleted the
jaygoss/new-688-sentry-outage-warning-block-incorrectly-shown-on-uptime
branch
The CronServiceIncidents overlay was being rendered on all detector types including uptime monitors. This moves the overlay rendering to a context prop (renderTimelineOverlay) that is only provided by cron detectors, ensuring it doesn't appear on uptime monitors.