diff --git a/tests/ui/job-view/PushListUrlDrift_test.jsx b/tests/ui/job-view/PushListUrlDrift_test.jsx new file mode 100644 index 00000000000..80c5e94edb5 --- /dev/null +++ b/tests/ui/job-view/PushListUrlDrift_test.jsx @@ -0,0 +1,178 @@ +import fetchMock from 'fetch-mock'; +import { BrowserRouter, useLocation } from 'react-router'; +import { render, waitFor, fireEvent, cleanup } from '@testing-library/react'; + +import { getProjectUrl } from '../../../ui/helpers/location'; +import FilterModel from '../../../ui/models/filter'; +import pushListFixture from '../mock/push_list'; +import jobListFixtureOne from '../mock/job_list/job_1'; +import PushList from '../../../ui/job-view/pushes/PushList'; +import { + usePushesStore, + fetchPushes, + initialState as pushesInitialState, +} from '../../../ui/shared/stores/pushesStore'; +import { getApiUrl } from '../../../ui/helpers/url'; + +// These tests use BrowserRouter (not MemoryRouter) on purpose: the bug under +// test is an interleaving of raw window.history.pushState calls (which React +// Router does NOT see) with popstate-dispatching URL updates (which it does). +// MemoryRouter can't reproduce that drift. + +const repoName = 'autoland'; +const push1Revision = 'ba9c692786e95143b8df3f4b3e9b504dfbc589a0'; // id 511138 + +// Router-visible search string, captured via a probe component. +let routerSearch; +function LocationProbe() { + routerSearch = useLocation().search; + return null; +} + +describe('PushList URL drift between window.location and React Router', () => { + const currentRepo = { + id: 4, + repository_group: { name: 'development', description: 'meh' }, + name: repoName, + dvcs_type: 'hg', + url: 'https://hg.mozilla.org/autoland', + branch: null, + codebase: 'gecko', + description: '', + active_status: 'active', + performance_alerts_enabled: false, + expire_performance_data: true, + is_try_repo: false, + pushLogUrl: 'https://hg.mozilla.org/autoland/pushloghtml', + revisionHrefPrefix: 'https://hg.mozilla.org/autoland/rev/', + getRevisionHref: () => 'foo', + getPushLogHref: () => 'foo', + }; + + // The URL fetchPushes builds when it falls back to a "reset" fetch after + // updateRange clears the store (fromchange/tochange present -> count=100). + const resetFetchUrl = `begin:${getProjectUrl( + '/push/?full=true&count=100', + repoName, + )}`; + + beforeAll(() => { + // Initial load of a single-revision view. + fetchMock.get( + getProjectUrl( + `/push/?full=true&count=10&revision=${push1Revision}`, + repoName, + ), + { ...pushListFixture, results: pushListFixture.results.slice(0, 1) }, + ); + // "get next 10" fetch (count is incremented to 11 with push_timestamp__lte). + fetchMock.get( + `begin:${getProjectUrl( + '/push/?full=true&count=11&push_timestamp__lte=', + repoName, + )}`, + { ...pushListFixture, results: pushListFixture.results.slice(1, 3) }, + ); + // The buggy full-range refetch after the phantom range change. + fetchMock.get(resetFetchUrl, { + ...pushListFixture, + results: pushListFixture.results.slice(0, 3), + }); + fetchMock.get(`begin:${getApiUrl('/jobs/?push_id=', repoName)}`, { + ...jobListFixtureOne, + }); + fetchMock.get( + 'begin:https://firefox-ci-tc.services.mozilla.com/api/index/v1/task/gecko.v2', + 404, + ); + fetchMock.get('begin:https://bugzilla.mozilla.org/rest/bug', { bugs: [] }); + }); + + beforeEach(() => { + window.history.replaceState( + null, + '', + `/jobs?repo=${repoName}&revision=${push1Revision}`, + ); + }); + + afterEach(() => { + cleanup(); + usePushesStore.setState({ ...pushesInitialState }); + }); + + afterAll(() => { + fetchMock.reset(); + }); + + const renderPushList = () => { + // Manually trigger fetchPushes since outside testing the App does it. + fetchPushes(); + + return render( + + +
+ {}} + /> +
+ , + ); + }; + + const pushCount = () => + document.querySelectorAll('[data-testid="push-header"]').length; + + test('"get next" keeps React Router location in sync with window.location', async () => { + const { getByTestId } = renderPushList(); + + await waitFor(() => expect(pushCount()).toBe(1)); + + fireEvent.click(getByTestId('get-next-10')); + await waitFor(() => expect(pushCount()).toBe(3)); + + // fetchNextPushes rewrites revision -> tochange and addPushes appends + // fromchange. React Router must see the same URL, otherwise its next + // navigation is computed from a stale search string. + await waitFor(() => expect(routerSearch).toBe(window.location.search)); + }); + + test('clicking empty push-list space after "get next" must not clear and refetch the push range', async () => { + const { getByTestId } = renderPushList(); + + await waitFor(() => expect(pushCount()).toBe(1)); + + fireEvent.click(getByTestId('get-next-10')); + await waitFor(() => expect(pushCount()).toBe(3)); + + // Sanity: the raw pushState calls rewrote the URL from revision=X to + // tochange=X&fromchange=Y. + expect(window.location.search).toContain('tochange='); + expect(window.location.search).not.toContain('revision='); + + // A mousedown on empty push-list space triggers clearJobViaUrl -> + // updateUrlSearch -> popstate. React Router wakes up, sees `revision` + // "disappear" relative to its stale location, and PushList treats that as + // a range change: clearPushes() + fetchPushes() -- wiping the pushes the + // user just loaded. + fireEvent.mouseDown(document.getElementById('push-list')); + + // Let the popstate-driven effects and any (buggy) refetch settle. + await new Promise((resolve) => { + setTimeout(resolve, 100); + }); + + // The user's effective push range did not change, so nothing should have + // been cleared or refetched. + expect(fetchMock.called(resetFetchUrl)).toBe(false); + expect(usePushesStore.getState().pushList).toHaveLength(3); + }); +}); diff --git a/tests/ui/job-view/PushList_test.jsx b/tests/ui/job-view/PushList_test.jsx index fa7dfab70a2..7602cede3f9 100644 --- a/tests/ui/job-view/PushList_test.jsx +++ b/tests/ui/job-view/PushList_test.jsx @@ -50,8 +50,9 @@ describe('PushList', () => { beforeEach(() => { mockNavigate = jest.fn(); - // Mock window.history.pushState for URL updates + // Mock window.history state updates so the jsdom URL stays clean jest.spyOn(window.history, 'pushState').mockImplementation(() => {}); + jest.spyOn(window.history, 'replaceState').mockImplementation(() => {}); }); const currentRepo = { diff --git a/tests/ui/job-view/stores/pushes_test.jsx b/tests/ui/job-view/stores/pushes_test.jsx index 356d3184717..acff6fdb7cd 100644 --- a/tests/ui/job-view/stores/pushes_test.jsx +++ b/tests/ui/job-view/stores/pushes_test.jsx @@ -30,6 +30,7 @@ describe('Pushes Zustand store', () => { fetchMock.get(getApiUrl('/jobs/?push_id=1', repoName), jobListFixtureOne); fetchMock.get(getApiUrl('/jobs/?push_id=2', repoName), jobListFixtureTwo); jest.spyOn(window.history, 'pushState').mockImplementation(() => {}); + jest.spyOn(window.history, 'replaceState').mockImplementation(() => {}); delete window.location; window.location = { ...originalLocation, search: '', pathname: '/jobs' }; // Reset store to initial state before each test @@ -147,9 +148,9 @@ describe('Pushes Zustand store', () => { await usePushesStore.getState().fetchPushes(10, true); - expect(window.history.pushState).toHaveBeenCalledWith( - null, - null, + expect(window.history.replaceState).toHaveBeenCalledWith( + {}, + '', expect.stringContaining( 'tochange=ba9c692786e95143b8df3f4b3e9b504dfbc589a0', ), diff --git a/ui/helpers/location.js b/ui/helpers/location.js index d887bb7a0b4..0124c06d79f 100644 --- a/ui/helpers/location.js +++ b/ui/helpers/location.js @@ -1,4 +1,5 @@ import { thDefaultRepo } from './constants'; +import { replaceUrlSearch } from './router'; import { createQueryParams, getApiUrl } from './url'; export const getAllUrlParams = function getAllUrlParams( @@ -19,9 +20,8 @@ export const getRepo = function getRepo() { return getUrlParam('repo') || thDefaultRepo; }; -// This won't update the react router history object export const replaceLocation = function replaceLocation(params) { - window.history.pushState(null, null, createQueryParams(params)); + replaceUrlSearch(createQueryParams(params)); }; export const setUrlParam = function setUrlParam(field, value) { diff --git a/ui/job-view/pushes/PushList.jsx b/ui/job-view/pushes/PushList.jsx index 8b9a43d0959..adab35695c6 100644 --- a/ui/job-view/pushes/PushList.jsx +++ b/ui/job-view/pushes/PushList.jsx @@ -18,6 +18,7 @@ import { pollPushes, } from '../../shared/stores/pushesStore'; import { updatePushParams } from '../../helpers/location'; +import { updateUrlSearch } from '../../helpers/router'; import Push from './Push'; import PushLoadErrors from './PushLoadErrors'; @@ -124,7 +125,11 @@ function PushList({ const fetchNextPushes = useCallback( (count) => { const params = updatePushParams(location); - window.history.pushState(null, null, params); + // The revision -> tochange (or startdate removal) rewrite keeps the same + // top of range, so it must not be treated as a range change by the + // URL-watching effect below. + prevRouterSearch.current = params; + updateUrlSearch(params); fetchPushes(count, true); }, [fetchPushes, location],