From b16425529df706b75801f64d8ae4b84c4e33db9d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Jul 2023 13:28:56 -0600 Subject: [PATCH 1/8] Prefetch neighboring files when in single-file file-by-file mode --- .../diffs/components/diff_file.vue | 7 + app/assets/javascripts/diffs/store/actions.js | 50 +++++ .../javascripts/diffs/store/mutation_types.js | 1 + .../javascripts/diffs/store/mutations.js | 3 + app/assets/javascripts/diffs/store/utils.js | 1 + .../diffs/utils/tree_worker_utils.js | 1 + .../diffs/components/diff_file_spec.js | 27 ++- spec/frontend/diffs/store/actions_spec.js | 198 ++++++++++++++++++ spec/frontend/diffs/store/mutations_spec.js | 30 +++ spec/frontend/diffs/store/utils_spec.js | 4 +- .../diffs/utils/tree_worker_utils_spec.js | 6 + 11 files changed, 325 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index 4e1ccfc530e95d..d044a878b14033 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -226,6 +226,12 @@ export default { } this.manageViewedEffects(); + + if (this.viewDiffsFileByFile) { + requestIdleCallback(() => { + this.prefetchFileNeighbors(); + }); + } }, beforeDestroy() { eventHub.$off(EVT_EXPAND_ALL_FILES, this.expandAllListener); @@ -234,6 +240,7 @@ export default { ...mapActions('diffs', [ 'loadCollapsedDiff', 'assignDiscussionsToDiff', + 'prefetchFileNeighbors', 'setFileCollapsedByUser', 'saveDiffDiscussion', 'toggleFileCommentForm', diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 2a557017953990..91c0a4c94a505e 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -113,6 +113,45 @@ export const setBaseConfig = ({ commit }, options) => { }); }; +export const prefetchSingleFile = async ({ state, getters, commit }, treeEntry) => { + const versionPath = state.mergeRequestDiff?.version_path; + + if ( + treeEntry && + !treeEntry.diffLoaded && + !treeEntry.diffLoading && + !getters.getDiffFileByHash(treeEntry.fileHash) + ) { + const urlParams = { + old_path: treeEntry.filePaths.old, + new_path: treeEntry.filePaths.new, + w: state.showWhitespace ? '0' : '1', + view: 'inline', + commit_id: getters.commitId, + }; + + if (versionPath) { + const { diffId, startSha } = getDerivedMergeRequestInformation({ endpoint: versionPath }); + + urlParams.diff_id = diffId; + urlParams.start_sha = startSha; + } + + commit(types.TREE_ENTRY_DIFF_LOADING, { path: treeEntry.filePaths.new }); + + axios + .get(mergeUrlParams({ ...urlParams }, state.endpointDiffForPath)) + .then(({ data: diffData }) => { + commit(types.SET_DIFF_DATA_BATCH, { diff_files: diffData.diff_files }); + + eventHub.$emit('diffFilesModified'); + }) + .catch(() => { + commit(types.TREE_ENTRY_DIFF_LOADING, { path: treeEntry.filePaths.new, loading: false }); + }); + } +}; + export const fetchFileByFile = async ({ state, getters, commit }) => { const isNoteLink = isUrlHashNoteLink(window?.location?.hash); const id = parseUrlHashAsFileHash(window?.location?.hash, state.currentDiffFileId); @@ -304,6 +343,17 @@ export const fetchDiffFilesMeta = ({ commit, state }) => { } }); }; + +export function prefetchFileNeighbors({ getters, dispatch }) { + const { flatBlobsList: allBlobs, currentDiffIndex: currentIndex } = getters; + + const previous = Math.max(currentIndex - 1, 0); + const next = Math.min(allBlobs.length - 1, currentIndex + 1); + + dispatch('prefetchSingleFile', allBlobs[next]); + dispatch('prefetchSingleFile', allBlobs[previous]); +} + export const fetchCoverageFiles = ({ commit, state }) => { const coveragePoll = new Poll({ resource: { diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index c32d82faad0dab..3df491503a4352 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/app/assets/javascripts/diffs/store/mutation_types.js @@ -19,6 +19,7 @@ export const RENDER_FILE = 'RENDER_FILE'; export const SET_LINE_DISCUSSIONS_FOR_FILE = 'SET_LINE_DISCUSSIONS_FOR_FILE'; export const REMOVE_LINE_DISCUSSIONS_FOR_FILE = 'REMOVE_LINE_DISCUSSIONS_FOR_FILE'; export const TOGGLE_FOLDER_OPEN = 'TOGGLE_FOLDER_OPEN'; +export const TREE_ENTRY_DIFF_LOADING = 'TREE_ENTRY_DIFF_LOADING'; export const SET_SHOW_TREE_LIST = 'SET_SHOW_TREE_LIST'; export const SET_CURRENT_DIFF_FILE = 'SET_CURRENT_DIFF_FILE'; export const SET_DIFF_FILE_VIEWED = 'SET_DIFF_FILE_VIEWED'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 4855ca87e91173..402b1a82ca40ff 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -266,6 +266,9 @@ export default { [types.TOGGLE_FOLDER_OPEN](state, path) { state.treeEntries[path].opened = !state.treeEntries[path].opened; }, + [types.TREE_ENTRY_DIFF_LOADING](state, { path, loading = true }) { + state.treeEntries[path].diffLoading = loading; + }, [types.SET_SHOW_TREE_LIST](state, showTreeList) { state.showTreeList = showTreeList; }, diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 97dfd351e67a93..307c41a98f83a6 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -593,6 +593,7 @@ export function markTreeEntriesLoaded({ priorEntries, loadedFiles }) { if (entry) { entry.diffLoaded = true; + entry.diffLoading = false; } }); diff --git a/app/assets/javascripts/diffs/utils/tree_worker_utils.js b/app/assets/javascripts/diffs/utils/tree_worker_utils.js index 8689809cfa9d75..01a475f48d16d7 100644 --- a/app/assets/javascripts/diffs/utils/tree_worker_utils.js +++ b/app/assets/javascripts/diffs/utils/tree_worker_utils.js @@ -86,6 +86,7 @@ export const generateTreeList = (files) => { Object.assign(entry, { changed: true, diffLoaded: false, + diffLoading: false, filePaths: { old: file.old_path, new: file.new_path, diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js index db6cde883f30bc..42b96040a166a4 100644 --- a/spec/frontend/diffs/components/diff_file_spec.js +++ b/spec/frontend/diffs/components/diff_file_spec.js @@ -42,6 +42,7 @@ jest.mock('~/notes/mixins/diff_line_note_form', () => ({ Vue.use(Vuex); const saveDiffDiscussionMock = jest.fn(); +const prefetchFileNeighborsMock = jest.fn(); function changeViewer(store, index, { automaticallyCollapsed, manuallyCollapsed, name }) { const file = store.state.diffs.diffFiles[index]; @@ -91,6 +92,7 @@ function createComponent({ file, first = false, last = false, options = {}, prop const diffs = diffsModule(); diffs.actions = { ...diffs.actions, + prefetchFileNeighbors: prefetchFileNeighborsMock, saveDiffDiscussion: saveDiffDiscussionMock, }; @@ -155,19 +157,42 @@ const triggerSaveDraftNote = (wrapper, note, parent, error) => findNoteForm(wrapper).vm.$emit('handleFormUpdateAddToReview', note, false, parent, error); describe('DiffFile', () => { + let readableFile; let wrapper; let store; let axiosMock; beforeEach(() => { + readableFile = getReadableFile(); axiosMock = new MockAdapter(axios); - ({ wrapper, store } = createComponent({ file: getReadableFile() })); + ({ wrapper, store } = createComponent({ file: readableFile })); }); afterEach(() => { axiosMock.restore(); }); + describe('mounted', () => { + it.each` + description | not | fileByFile + ${'does not prefetch if not in file-by-file mode'} | ${true} | ${false} + ${'prefetches when in file-by-file mode with the single-file feature flag enabled'} | ${false} | ${true} + `('$description', ({ not, fileByFile }) => { + createComponent({ + props: { viewDiffsFileByFile: fileByFile }, + file: readableFile, + }); + + // This line allows `window.requestIdleCallback` to execute + jest.advanceTimersByTime(0); + + /* eslint-disable-next-line jest/valid-expect */ + const expectation = expect(prefetchFileNeighborsMock); + + (not ? expectation.not : expectation).toHaveBeenCalled(); + }); + }); + describe('bus events', () => { beforeEach(() => { jest.spyOn(eventHub, '$emit').mockImplementation(() => {}); diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index bbe748b8e1fc86..d8f86d9a22bf0a 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -147,6 +147,173 @@ describe('DiffsStoreActions', () => { }); }); + describe('prefetchSingleFile', () => { + beforeEach(() => { + window.location.hash = 'e334a2a10f036c00151a04cea7938a5d4213a818'; + }); + + it('should do nothing if the tree entry is already loading', () => { + return testAction(diffActions.prefetchSingleFile, { diffLoading: true }, {}, [], []); + }); + + it('should do nothing if the tree entry has already been marked as loaded', () => { + return testAction( + diffActions.prefetchSingleFile, + { diffLoaded: true }, + { + flatBlobsList: [ + { fileHash: 'e334a2a10f036c00151a04cea7938a5d4213a818', diffLoaded: true }, + ], + }, + [], + [], + ); + }); + + describe('when a tree entry exists for the file, but it has not been marked as loaded', () => { + let state; + let getters; + let commit; + let hubSpy; + const defaultParams = { + old_path: 'old/123', + new_path: 'new/123', + w: '1', + view: 'inline', + }; + const endpointDiffForPath = '/diffs/set/endpoint/path'; + const diffForPath = mergeUrlParams(defaultParams, endpointDiffForPath); + const treeEntry = { + fileHash: 'e334a2a10f036c00151a04cea7938a5d4213a818', + filePaths: { old: 'old/123', new: 'new/123' }, + }; + const fileResult = { + diff_files: [{ file_hash: 'e334a2a10f036c00151a04cea7938a5d4213a818' }], + }; + + beforeEach(() => { + commit = jest.fn(); + state = { + endpointDiffForPath, + diffFiles: [], + }; + getters = { + flatBlobsList: [treeEntry], + getDiffFileByHash(hash) { + return state.diffFiles?.find((entry) => entry.file_hash === hash); + }, + }; + hubSpy = jest.spyOn(diffsEventHub, '$emit'); + }); + + it('does nothing if the file already exists in the loaded diff files', () => { + state.diffFiles = fileResult.diff_files; + + return testAction(diffActions.prefetchSingleFile, treeEntry, getters, [], []); + }); + + it('does some standard work every time', async () => { + mock.onGet(diffForPath).reply(HTTP_STATUS_OK, fileResult); + + await diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + expect(commit).toHaveBeenCalledWith(types.TREE_ENTRY_DIFF_LOADING, { + path: treeEntry.filePaths.new, + }); + + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + expect(commit).toHaveBeenCalledWith(types.SET_DIFF_DATA_BATCH, fileResult); + + expect(hubSpy).toHaveBeenCalledWith('diffFilesModified'); + }); + + it('should fetch data without commit ID', async () => { + getters.commitId = null; + mock.onGet(diffForPath).reply(HTTP_STATUS_OK, fileResult); + + await diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + // This tests that commit_id is NOT added, if there isn't one in the store + expect(mock.history.get[0].url).toEqual(diffForPath); + }); + + it('should fetch data with commit ID', async () => { + const finalPath = mergeUrlParams( + { ...defaultParams, commit_id: '123' }, + endpointDiffForPath, + ); + + getters.commitId = '123'; + mock.onGet(finalPath).reply(HTTP_STATUS_OK, fileResult); + + await diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + expect(mock.history.get[0].url).toEqual(finalPath); + }); + + describe('version parameters', () => { + const diffId = '4'; + const startSha = 'abc'; + const pathRoot = 'a/a/-/merge_requests/1'; + + it('fetches the data when there is no mergeRequestDiff', async () => { + diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + expect(mock.history.get[0].url).toEqual(diffForPath); + }); + + it.each` + desc | versionPath | start_sha | diff_id + ${'no additional version information'} | ${`${pathRoot}?search=terms`} | ${undefined} | ${undefined} + ${'the diff_id'} | ${`${pathRoot}?diff_id=${diffId}`} | ${undefined} | ${diffId} + ${'the start_sha'} | ${`${pathRoot}?start_sha=${startSha}`} | ${startSha} | ${undefined} + ${'all available version information'} | ${`${pathRoot}?diff_id=${diffId}&start_sha=${startSha}`} | ${startSha} | ${diffId} + `('fetches the data and includes $desc', async ({ versionPath, start_sha, diff_id }) => { + const finalPath = mergeUrlParams( + { ...defaultParams, diff_id, start_sha }, + endpointDiffForPath, + ); + state.mergeRequestDiff = { version_path: versionPath }; + mock.onGet(finalPath).reply(HTTP_STATUS_OK, fileResult); + + diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + expect(mock.history.get[0].url).toEqual(finalPath); + }); + }); + + describe('when the prefetch fails', () => { + it('should commit a mutation to set the tree entry diff loading to false', async () => { + mock.onGet(diffForPath).reply(HTTP_STATUS_INTERNAL_SERVER_ERROR); + + diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + expect(commit).toHaveBeenCalledWith(types.TREE_ENTRY_DIFF_LOADING, { + path: treeEntry.filePaths.new, + loading: false, + }); + }); + }); + }); + }); + describe('fetchFileByFile', () => { beforeEach(() => { window.location.hash = 'e334a2a10f036c00151a04cea7938a5d4213a818'; @@ -460,6 +627,37 @@ describe('DiffsStoreActions', () => { }); }); + describe('prefetchFileNeighbors', () => { + it('dispatches two requests to prefetch the next/previous files', () => { + testAction( + diffActions.prefetchFileNeighbors, + {}, + { + currentDiffIndex: 0, + flatBlobsList: [ + { + type: 'blob', + fileHash: 'abc', + }, + { + type: 'blob', + fileHash: 'def', + }, + { + type: 'blob', + fileHash: 'ghi', + }, + ], + }, + [], + [ + { type: 'prefetchSingleFile', payload: { type: 'blob', fileHash: 'def' } }, + { type: 'prefetchSingleFile', payload: { type: 'blob', fileHash: 'abc' } }, + ], + ); + }); + }); + describe('fetchCoverageFiles', () => { const endpointCoverage = '/fetch'; diff --git a/spec/frontend/diffs/store/mutations_spec.js b/spec/frontend/diffs/store/mutations_spec.js index 274cb40dac8e73..e87c5d0a9b1e23 100644 --- a/spec/frontend/diffs/store/mutations_spec.js +++ b/spec/frontend/diffs/store/mutations_spec.js @@ -684,6 +684,36 @@ describe('DiffsStoreMutations', () => { }); }); + describe('TREE_ENTRY_DIFF_LOADING', () => { + it('sets the entry loading state to true by default', () => { + const state = { + treeEntries: { + path: { + diffLoading: false, + }, + }, + }; + + mutations[types.TREE_ENTRY_DIFF_LOADING](state, { path: 'path' }); + + expect(state.treeEntries.path.diffLoading).toBe(true); + }); + + it('sets the entry loading state to the provided value', () => { + const state = { + treeEntries: { + path: { + diffLoading: true, + }, + }, + }; + + mutations[types.TREE_ENTRY_DIFF_LOADING](state, { path: 'path', loading: false }); + + expect(state.treeEntries.path.diffLoading).toBe(false); + }); + }); + describe('SET_SHOW_TREE_LIST', () => { it('sets showTreeList', () => { const state = createState(); diff --git a/spec/frontend/diffs/store/utils_spec.js b/spec/frontend/diffs/store/utils_spec.js index 117ed56e3479e0..24cb8158739c8e 100644 --- a/spec/frontend/diffs/store/utils_spec.js +++ b/spec/frontend/diffs/store/utils_spec.js @@ -948,9 +948,9 @@ describe('DiffsStoreUtils', () => { describe('markTreeEntriesLoaded', () => { it.each` desc | entries | loaded | outcome - ${'marks an existing entry as loaded'} | ${{ abc: {} }} | ${[{ new_path: 'abc' }]} | ${{ abc: { diffLoaded: true } }} + ${'marks an existing entry as loaded'} | ${{ abc: {} }} | ${[{ new_path: 'abc' }]} | ${{ abc: { diffLoaded: true, diffLoading: false } }} ${'does nothing if the new file is not found in the tree entries'} | ${{ abc: {} }} | ${[{ new_path: 'def' }]} | ${{ abc: {} }} - ${'leaves entries unmodified if they are not in the loaded files'} | ${{ abc: {}, def: { diffLoaded: true }, ghi: {} }} | ${[{ new_path: 'ghi' }]} | ${{ abc: {}, def: { diffLoaded: true }, ghi: { diffLoaded: true } }} + ${'leaves entries unmodified if they are not in the loaded files'} | ${{ abc: {}, def: { diffLoaded: true }, ghi: {} }} | ${[{ new_path: 'ghi' }]} | ${{ abc: {}, def: { diffLoaded: true }, ghi: { diffLoaded: true, diffLoading: false } }} `('$desc', ({ entries, loaded, outcome }) => { expect(utils.markTreeEntriesLoaded({ priorEntries: entries, loadedFiles: loaded })).toEqual( outcome, diff --git a/spec/frontend/diffs/utils/tree_worker_utils_spec.js b/spec/frontend/diffs/utils/tree_worker_utils_spec.js index b8bd4fcd0814a6..5fba00f9258acc 100644 --- a/spec/frontend/diffs/utils/tree_worker_utils_spec.js +++ b/spec/frontend/diffs/utils/tree_worker_utils_spec.js @@ -76,6 +76,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { addedLines: 0, changed: true, diffLoaded: false, + diffLoading: false, deleted: false, fileHash: 'test', filePaths: { @@ -103,6 +104,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { addedLines: 0, changed: true, diffLoaded: false, + diffLoading: false, deleted: false, fileHash: 'test', filePaths: { @@ -123,6 +125,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { addedLines: 0, changed: true, diffLoaded: false, + diffLoading: false, deleted: false, fileHash: 'test', filePaths: { @@ -154,6 +157,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { addedLines: 42, changed: true, diffLoaded: false, + diffLoading: false, deleted: false, fileHash: 'test', filePaths: { @@ -181,6 +185,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { type: 'blob', changed: true, diffLoaded: false, + diffLoading: false, tempFile: true, submodule: true, deleted: false, @@ -201,6 +206,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { type: 'blob', changed: true, diffLoaded: false, + diffLoading: false, tempFile: false, submodule: undefined, deleted: true, -- GitLab From 99d5dfc56c605fa80174632b89f9100e1ace1e2e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 18 Jul 2023 09:45:39 -0600 Subject: [PATCH 2/8] (Review) use try/catch instead of .then/.catch --- app/assets/javascripts/diffs/store/actions.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 91c0a4c94a505e..c3dc4a198e7931 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -139,16 +139,17 @@ export const prefetchSingleFile = async ({ state, getters, commit }, treeEntry) commit(types.TREE_ENTRY_DIFF_LOADING, { path: treeEntry.filePaths.new }); - axios - .get(mergeUrlParams({ ...urlParams }, state.endpointDiffForPath)) - .then(({ data: diffData }) => { - commit(types.SET_DIFF_DATA_BATCH, { diff_files: diffData.diff_files }); + try { + const { data: diffData } = await axios.get( + mergeUrlParams({ ...urlParams }, state.endpointDiffForPath), + ); - eventHub.$emit('diffFilesModified'); - }) - .catch(() => { - commit(types.TREE_ENTRY_DIFF_LOADING, { path: treeEntry.filePaths.new, loading: false }); - }); + commit(types.SET_DIFF_DATA_BATCH, { diff_files: diffData.diff_files }); + + eventHub.$emit('diffFilesModified'); + } catch (e) { + commit(types.TREE_ENTRY_DIFF_LOADING, { path: treeEntry.filePaths.new, loading: false }); + } } }; -- GitLab From 70621511f0c786cc555c36e3e17750b876ae1649 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 18 Jul 2023 09:46:00 -0600 Subject: [PATCH 3/8] (Review) Fix test name --- spec/frontend/diffs/components/diff_file_spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js index 42b96040a166a4..eb993e0defd728 100644 --- a/spec/frontend/diffs/components/diff_file_spec.js +++ b/spec/frontend/diffs/components/diff_file_spec.js @@ -174,9 +174,9 @@ describe('DiffFile', () => { describe('mounted', () => { it.each` - description | not | fileByFile - ${'does not prefetch if not in file-by-file mode'} | ${true} | ${false} - ${'prefetches when in file-by-file mode with the single-file feature flag enabled'} | ${false} | ${true} + description | not | fileByFile + ${'does not prefetch if not in file-by-file mode'} | ${true} | ${false} + ${'prefetches when in file-by-file mode'} | ${false} | ${true} `('$description', ({ not, fileByFile }) => { createComponent({ props: { viewDiffsFileByFile: fileByFile }, -- GitLab From 2742d1bda3e581583ed761ca4c70c8c56d3b971b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 18 Jul 2023 09:46:59 -0600 Subject: [PATCH 4/8] (Review) Mock `requestIdleCallback` instead of using pre-mocked timers --- spec/frontend/diffs/components/diff_file_spec.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js index eb993e0defd728..32d11b279ee149 100644 --- a/spec/frontend/diffs/components/diff_file_spec.js +++ b/spec/frontend/diffs/components/diff_file_spec.js @@ -173,6 +173,10 @@ describe('DiffFile', () => { }); describe('mounted', () => { + beforeEach(() => { + jest.spyOn(window, 'requestIdleCallback').mockImplementation((fn) => fn()); + }); + it.each` description | not | fileByFile ${'does not prefetch if not in file-by-file mode'} | ${true} | ${false} @@ -183,9 +187,6 @@ describe('DiffFile', () => { file: readableFile, }); - // This line allows `window.requestIdleCallback` to execute - jest.advanceTimersByTime(0); - /* eslint-disable-next-line jest/valid-expect */ const expectation = expect(prefetchFileNeighborsMock); -- GitLab From eb46d20eec60f42665a0cad15b2ddcf3e4c35cb8 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 18 Jul 2023 09:47:33 -0600 Subject: [PATCH 5/8] (Review) Move endpoint mock to `beforeEach` --- spec/frontend/diffs/store/actions_spec.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index d8f86d9a22bf0a..ab10ddfc675441 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -297,9 +297,11 @@ describe('DiffsStoreActions', () => { }); describe('when the prefetch fails', () => { - it('should commit a mutation to set the tree entry diff loading to false', async () => { + beforeEach(() => { mock.onGet(diffForPath).reply(HTTP_STATUS_INTERNAL_SERVER_ERROR); + }); + it('should commit a mutation to set the tree entry diff loading to false', async () => { diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); // wait for the mocked network request to return and start processing the .then -- GitLab From 36d57629c600bcab257f48a5633a2619247b25e0 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 18 Jul 2023 09:48:26 -0600 Subject: [PATCH 6/8] (Review) Stop using ternary & infer expectation from feature setup --- .../frontend/diffs/components/diff_file_spec.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js index 32d11b279ee149..d0b490198bc85c 100644 --- a/spec/frontend/diffs/components/diff_file_spec.js +++ b/spec/frontend/diffs/components/diff_file_spec.js @@ -178,19 +178,20 @@ describe('DiffFile', () => { }); it.each` - description | not | fileByFile - ${'does not prefetch if not in file-by-file mode'} | ${true} | ${false} - ${'prefetches when in file-by-file mode'} | ${false} | ${true} - `('$description', ({ not, fileByFile }) => { + description | fileByFile + ${'does not prefetch if not in file-by-file mode'} | ${false} + ${'prefetches when in file-by-file mode'} | ${true} + `('$description', ({ fileByFile }) => { createComponent({ props: { viewDiffsFileByFile: fileByFile }, file: readableFile, }); - /* eslint-disable-next-line jest/valid-expect */ - const expectation = expect(prefetchFileNeighborsMock); - - (not ? expectation.not : expectation).toHaveBeenCalled(); + if (fileByFile) { + expect(prefetchFileNeighborsMock).toHaveBeenCalled(); + } else { + expect(prefetchFileNeighborsMock).not.toHaveBeenCalled(); + } }); }); -- GitLab From 63b2f131018aedff61531bab76d34c5181f933b2 Mon Sep 17 00:00:00 2001 From: Laura Meckley Date: Wed, 19 Jul 2023 09:09:32 +0000 Subject: [PATCH 7/8] Apply 3 suggestion(s) to 1 file(s) --- spec/frontend/diffs/store/actions_spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index ab10ddfc675441..3dc56f1fac6c90 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -221,7 +221,7 @@ describe('DiffsStoreActions', () => { path: treeEntry.filePaths.new, }); - // wait for the mocked network request to return and start processing the .then + // wait for the mocked network request to return await waitForPromises(); expect(commit).toHaveBeenCalledWith(types.SET_DIFF_DATA_BATCH, fileResult); @@ -289,7 +289,7 @@ describe('DiffsStoreActions', () => { diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); - // wait for the mocked network request to return and start processing the .then + // wait for the mocked network request to return await waitForPromises(); expect(mock.history.get[0].url).toEqual(finalPath); @@ -304,7 +304,7 @@ describe('DiffsStoreActions', () => { it('should commit a mutation to set the tree entry diff loading to false', async () => { diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); - // wait for the mocked network request to return and start processing the .then + // wait for the mocked network request to return await waitForPromises(); expect(commit).toHaveBeenCalledWith(types.TREE_ENTRY_DIFF_LOADING, { -- GitLab From 79d15615bb0805fc3b3a9f75f19927960d801523 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 19 Jul 2023 10:02:13 -0600 Subject: [PATCH 8/8] Remove trailing space from applied suggestions --- spec/frontend/diffs/store/actions_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 3dc56f1fac6c90..387407a7e4d6cb 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -221,7 +221,7 @@ describe('DiffsStoreActions', () => { path: treeEntry.filePaths.new, }); - // wait for the mocked network request to return + // wait for the mocked network request to return await waitForPromises(); expect(commit).toHaveBeenCalledWith(types.SET_DIFF_DATA_BATCH, fileResult); -- GitLab