Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor public API functions to no longer import and use store. #134

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions src/api.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,28 @@
import { examRequiresAccessToken, store } from './data';
import { useDispatch, useSelector } from 'react-redux';
import { examRequiresAccessToken } from './data';

export const useIsExam = () => {
const { exam } = useSelector(state => state.specialExams);
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious about renaming this function. Is the intent to express that a selector is being used? I feel like the previous names expressed what the function was doing in a more clear way (i.e. what it returns).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. I agree that isExam is a better name than useIsExam, but because I've turned an ordinary JavaScript function into a React hook, and because hooks names always start with use, I have to prefix the function name with use.


export function isExam() {
const { exam } = store.getState().specialExams;
return !!exam?.id;
}
};

export const useExamAccessToken = () => {
const { exam, examAccessToken } = useSelector(state => state.specialExams);

export function getExamAccess() {
const { exam, examAccessToken } = store.getState().specialExams;
if (!exam) {
return '';
}

return examAccessToken.exam_access_token;
}
};

export const useFetchExamAccessToken = () => {
const { exam } = useSelector(state => state.specialExams);
const dispatch = useDispatch();

export async function fetchExamAccess() {
const { exam } = store.getState().specialExams;
const { dispatch } = store;
if (!exam) {
return Promise.resolve();
}
return dispatch(examRequiresAccessToken());
}
return () => dispatch(examRequiresAccessToken());
};
62 changes: 41 additions & 21 deletions src/api.test.jsx
Original file line number Diff line number Diff line change
@@ -1,60 +1,80 @@
import { Factory } from 'rosie';

import { isExam, getExamAccess, fetchExamAccess } from './api';
import { store } from './data';
import { useExamAccessToken, useFetchExamAccessToken, useIsExam } from './api';
import { initializeTestStore, render } from './setupTest';

/**
* Hooks must be run in the scope of a component. To run the hook, wrap it in a test component whose sole
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding in this comment - super helpful!

* responsibility it is to run the hook and assign it to a return value that is returned by the function.
* @param {*} hook: the hook function to run
* @param {*} store: an initial store, passed to the call to render
* @returns: the return value of the hook
*/
const getHookReturnValue = (hook, store) => {
let returnVal;
const TestComponent = () => {
returnVal = hook();
return null;
};
render(<TestComponent />, { store });
return returnVal;
};

describe('External API integration tests', () => {
describe('Test isExam with exam', () => {
describe('Test useIsExam with exam', () => {
let store;

beforeAll(() => {
jest.mock('./data');
const mockExam = Factory.build('exam', { attempt: Factory.build('attempt') });
const mockToken = Factory.build('examAccessToken');
const mockState = { specialExams: { exam: mockExam, examAccessToken: mockToken } };
store.getState = jest.fn().mockReturnValue(mockState);
store = initializeTestStore(mockState);
});

afterAll(() => {
jest.clearAllMocks();
jest.resetAllMocks();
});

it('isExam should return true if exam is set', () => {
expect(isExam()).toBe(true);
it('useIsExam should return true if exam is set', () => {
expect(getHookReturnValue(useIsExam, store)).toBe(true);
});

it('getExamAccess should return exam access token if access token', () => {
expect(getExamAccess()).toBeTruthy();
it('useExamAccessToken should return exam access token if access token', () => {
expect(getHookReturnValue(useExamAccessToken, store)).toBeTruthy();
});

it('fetchExamAccess should dispatch get exam access token', () => {
const dispatchReturn = fetchExamAccess();
expect(dispatchReturn).toBeInstanceOf(Promise);
it('useFetchExamAccessToken should dispatch get exam access token', () => {
// The useFetchExamAccessToken hook returns a function that calls dispatch, so we must call the returned
// value to invoke dispatch.
expect(getHookReturnValue(useFetchExamAccessToken, store)()).toBeInstanceOf(Promise);
});
});

describe('Test isExam without exam', () => {
describe('Test useIsExam without exam', () => {
let store;

beforeAll(() => {
jest.mock('./data');
const mockState = { specialExams: { exam: null, examAccessToken: null } };
store.getState = jest.fn().mockReturnValue(mockState);
store = initializeTestStore(mockState);
});

afterAll(() => {
jest.clearAllMocks();
jest.resetAllMocks();
});

it('isExam should return false if exam is not set', () => {
expect(isExam()).toBe(false);
it('useIsExam should return false if exam is not set', () => {
expect(getHookReturnValue(useIsExam, store)).toBe(false);
});

it('getExamAccess should return empty string if exam access token not set', () => {
expect(getExamAccess()).toBeFalsy();
it('useExamAccessToken should return empty string if exam access token not set', () => {
expect(getHookReturnValue(useExamAccessToken, store)).toBeFalsy();
});

it('fetchExamAccess should not dispatch get exam access token', () => {
const dispatchReturn = fetchExamAccess();
expect(dispatchReturn).toBeInstanceOf(Promise);
it('useFetchExamAccessToken should not dispatch get exam access token', () => {
expect(getHookReturnValue(useFetchExamAccessToken, store)).toBeInstanceOf(Promise);
});
});
});
4 changes: 2 additions & 2 deletions src/core/OuterExamTimer.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ describe('OuterExamTimer', () => {

let store;

beforeEach(async () => {
store = await initializeTestStore();
beforeEach(() => {
store = initializeTestStore();
});

it('is successfully rendered and shows timer if there is an exam in progress', () => {
Expand Down
2 changes: 2 additions & 0 deletions src/data/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ export {
checkExamEntry,
} from './thunks';

export { default as reducer } from './slice';

export {
expireExamAttempt,
} from './slice';
Expand Down
4 changes: 2 additions & 2 deletions src/data/redux.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@ describe('Data layer integration tests', () => {
await executeThunk(thunks.getExamAttemptsData(courseId, contentId), store.dispatch);
};

beforeEach(async () => {
beforeEach(() => {
initializeTestConfig();
windowSpy = jest.spyOn(window, 'window', 'get');
axiosMock.reset();
loggingService.logError.mockReset();
loggingService.logInfo.mockReset();
store = await initializeTestStore();
store = initializeTestStore();
});

afterEach(() => {
Expand Down
4 changes: 2 additions & 2 deletions src/exam/ExamAPIError.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ describe('ExamAPIError', () => {

let store;

beforeEach(async () => {
store = await initializeTestStore();
beforeEach(() => {
store = initializeTestStore();
});

it('renders with the default information', () => {
Expand Down
18 changes: 5 additions & 13 deletions src/exam/ExamWrapper.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,16 @@ import '@testing-library/jest-dom';
import { Factory } from 'rosie';
import React from 'react';
import SequenceExamWrapper from './ExamWrapper';
import { startTimedExam } from '../data';
import { getExamAttemptsData } from '../data/thunks';
import { getExamAttemptsData, startTimedExam } from '../data';
import { render, waitFor, initializeTestStore } from '../setupTest';
import { ExamStatus, ExamType } from '../constants';

jest.mock('../data', () => ({
store: {},
startTimedExam: jest.fn(),
}));

// because of the way ExamStateProvider and other locations inconsistantly import from
// thunks directly instead of using the data module we need to mock the underlying
// thunk file. It would be nice to clean this up in the future.
jest.mock('../data/thunks', () => {
jest.mock('../data', () => {
const originalModule = jest.requireActual('../data/thunks');
return {
...originalModule,
getExamAttemptsData: jest.fn(),
startTimedExam: jest.fn(),
};
});

Expand All @@ -34,9 +26,9 @@ describe('SequenceExamWrapper', () => {
const courseId = 'course-v1:test+test+test';
let store;

beforeEach(async () => {
beforeEach(() => {
jest.clearAllMocks();
store = await initializeTestStore({
store = initializeTestStore({
specialExams: Factory.build('specialExams'),
isLoading: false,
});
Expand Down
7 changes: 4 additions & 3 deletions src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
export { default } from './core/SequenceExamWrapper';
export { default as OuterExamTimer } from './core/OuterExamTimer';
export {
getExamAccess,
isExam,
fetchExamAccess,
useExamAccessToken,
useFetchExamAccessToken,
useIsExam,
} from './api';
export { reducer } from './data';
4 changes: 2 additions & 2 deletions src/instructions/Instructions.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ pollExamAttempt.mockReturnValue(Promise.resolve({}));
describe('SequenceExamWrapper', () => {
let store;

beforeEach(async () => {
beforeEach(() => {
initializeMockApp();
store = await initializeTestStore();
store = initializeTestStore();
store.subscribe = jest.fn();
store.dispatch = jest.fn();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ getExamAttemptsData.mockReturnValue(jest.fn());
describe('SequenceExamWrapper', () => {
let store;

beforeEach(async () => {
beforeEach(() => {
initializeMockApp();
jest.clearAllMocks();
store = await initializeTestStore();
store = initializeTestStore();
store.subscribe = jest.fn();
store.dispatch = jest.fn();
});
Expand Down
2 changes: 1 addition & 1 deletion src/setupTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export function initializeMockApp() {

let globalStore;

export async function initializeTestStore(preloadedState = null, overrideStore = true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why this was marked async. There's nothing asynchronous about it. It might have been copy pasted from frontend-app-learning's initializeTestStore function, which does have asynchronicity.

export function initializeTestStore(preloadedState = null, overrideStore = true) {
let store = configureStore({
reducer: {
specialExams: examReducer,
Expand Down
22 changes: 11 additions & 11 deletions src/timer/CountDownTimer.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('ExamTimerBlock', () => {
submitExam.mockReturnValue(jest.fn());
stopExam.mockReturnValue(jest.fn());

beforeEach(async () => {
beforeEach(() => {
const preloadedState = {
specialExams: {
isLoading: true,
Expand All @@ -44,7 +44,7 @@ describe('ExamTimerBlock', () => {
},
},
};
store = await initializeTestStore(preloadedState);
store = initializeTestStore(preloadedState);
attempt = store.getState().specialExams.activeAttempt;
});

Expand All @@ -60,7 +60,7 @@ describe('ExamTimerBlock', () => {
expect(screen.getByRole('button', { name: 'End My Exam' })).toBeInTheDocument();
});

it('renders without activeAttempt return null', async () => {
it('renders without activeAttempt return null', () => {
const preloadedState = {
specialExams: {
isLoading: true,
Expand All @@ -70,7 +70,7 @@ describe('ExamTimerBlock', () => {
exam: {},
},
};
const testStore = await initializeTestStore(preloadedState);
const testStore = initializeTestStore(preloadedState);
attempt = testStore.getState().specialExams.activeAttempt;
const { container } = render(
<ExamTimerBlock />,
Expand Down Expand Up @@ -106,7 +106,7 @@ describe('ExamTimerBlock', () => {
},
},
};
const testStore = await initializeTestStore(preloadedState);
const testStore = initializeTestStore(preloadedState);
attempt = testStore.getState().specialExams.activeAttempt;
render(
<ExamTimerBlock />,
Expand Down Expand Up @@ -168,7 +168,7 @@ describe('ExamTimerBlock', () => {
},
},
};
const testStore = await initializeTestStore(preloadedState);
const testStore = initializeTestStore(preloadedState);
attempt = testStore.getState().specialExams.activeAttempt;

render(
Expand Down Expand Up @@ -210,7 +210,7 @@ describe('ExamTimerBlock', () => {
},
},
};
let testStore = await initializeTestStore(preloadedState);
let testStore = initializeTestStore(preloadedState);
attempt = testStore.getState().specialExams.activeAttempt;
const { rerender } = render(
<ExamTimerBlock />,
Expand All @@ -221,7 +221,7 @@ describe('ExamTimerBlock', () => {
...attempt,
time_remaining_seconds: 20,
};
testStore = await initializeTestStore(preloadedState);
testStore = initializeTestStore(preloadedState);
const updatedAttempt = testStore.getState().specialExams.activeAttempt;

expect(updatedAttempt.time_remaining_seconds).toBe(20);
Expand All @@ -245,7 +245,7 @@ describe('ExamTimerBlock', () => {
'30 minutes': 1800,
};
Object.keys(timesToTest).forEach((timeString) => {
it(`Accessibility time string ${timeString} appears as expected based seconds remaining: ${timesToTest[timeString]}`, async () => {
it(`Accessibility time string ${timeString} appears as expected based seconds remaining: ${timesToTest[timeString]}`, () => {
// create a state with the respective number of seconds
const preloadedState = {
specialExams: {
Expand All @@ -268,7 +268,7 @@ describe('ExamTimerBlock', () => {
};

// Store it in the state
const testStore = await initializeTestStore(preloadedState);
const testStore = initializeTestStore(preloadedState);
attempt = testStore.getState().specialExams.activeAttempt;

// render an exam timer block with that data
Expand All @@ -277,7 +277,7 @@ describe('ExamTimerBlock', () => {
);

// expect the a11y string to be a certain output
await waitFor(() => expect(screen.getByText(`you have ${timeString} remaining`)).toBeInTheDocument());
expect(screen.getByText(`you have ${timeString} remaining`)).toBeInTheDocument();
});
});
});
Loading