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

test: add testing to useParagonTheme hooks #514

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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
106 changes: 106 additions & 0 deletions src/react/hooks/paragon/useParagonThemeCore.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
import { renderHook, act } from '@testing-library/react-hooks';
import { getConfig } from '../../../config';
import { logError } from '../../../logging';
import useParagonThemeCore from './useParagonThemeCore';

jest.mock('../../../logging');

describe('useParagonThemeCore', () => {
const themeOnLoad = jest.fn();
let initialState = false;

afterEach(() => {
initialState = false;
document.head.innerHTML = '';
themeOnLoad.mockClear();
logError.mockClear();
Copy link
Member

Choose a reason for hiding this comment

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

nit: could use jest.clearAllMocks()

});
it('should load the core url and change the loading state to true', () => {
themeOnLoad.mockImplementation(() => { initialState = true; });
const coreConfig = {
themeCore: {
urls: { default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/core.min.css' },
},
onLoad: themeOnLoad,
};
renderHook(() => useParagonThemeCore(coreConfig));
const createdLinkTag = document.head.querySelector('link');
act(() => createdLinkTag.onload());
expect(createdLinkTag.href).toBe(coreConfig.themeCore.urls.default);
expect(themeOnLoad).toHaveBeenCalledTimes(1);
expect(initialState).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

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

[clarification/curious] What is the intent of the initialState assertion? Given you assert that themeOnLoad is called once on line 30 already, do we need to assert on something that only happens in the test? Put another way, does this hook need to know what onLoad does or does it only care that onLoad is called when appropriate?

Similar questions for other uses of initialState in other tests below.

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 agree, due to is a parameter and not a hook method shouldn't test its functionality (out of the scope).

});

it('should load the core default and brand url and change the loading state to true', () => {
themeOnLoad.mockImplementation(() => { initialState = true; });
const coreConfig = {
themeCore: {
urls: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/core.min.css',
brandOverride: 'https://cdn.jsdelivr.net/npm/@edx/brand@$brandVersionVersion/dist/core.min.css',
Copy link
Member

Choose a reason for hiding this comment

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

nit: might replace $paragonVersion and $brandVersion with explicit version numbers and/or a distribution tag like alpha (e.g., https://cdn.jsdelivr.net/npm/@edx/paragon@alpha/dist/core.min.css). The URLs passed into this hook have already had their wildcard keywords replaced.

},
},
onLoad: themeOnLoad,
};
renderHook(() => useParagonThemeCore(coreConfig));
const createdLinkTag = document.head.querySelector('link[data-paragon-theme-core="true"]');
const createdBrandLinkTag = document.head.querySelector('link[data-brand-theme-core="true"]');

act(() => { createdLinkTag.onload(); createdBrandLinkTag.onload(); });
expect(createdLinkTag.href).toBe(coreConfig.themeCore.urls.default);
expect(createdBrandLinkTag.href).toBe(coreConfig.themeCore.urls.brandOverride);
expect(themeOnLoad).toHaveBeenCalledTimes(1);
expect(initialState).toBeTruthy();
});

it('should dispatch a log error and fallback to PARAGON_THEME if can not load the core theme link', () => {
global.PARAGON_THEME = {
paragon: {
version: '1.0.0',
themeUrls: {
core: {
fileName: 'core.min.css',
},
variants: {
light: {
fileName: 'light.min.css',
default: true,
dark: false,
Copy link
Member

Choose a reason for hiding this comment

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

This config schema has changed on the base branch, e.g.:

global.PARAGON_THEME = {
      paragon: {
        version: '1.0.0',
        themeUrls: {
          core: {
            fileName: 'core.min.css',
          },
          defaults: {
            light: 'light',
          },
          variants: {
            light: {
              fileName: 'light.min.css',
            },
          },
        },
      },
    };

},
},
},
},
};
themeOnLoad.mockImplementation(() => { initialState = true; });
const coreConfig = {
themeCore: {
urls: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/core.min.css',
},
},
onLoad: themeOnLoad,
};
renderHook(() => useParagonThemeCore(coreConfig));
const createdLinkTag = document.head.querySelector('link[data-paragon-theme-core="true"]');

act(() => { createdLinkTag.onerror(); });
expect(logError).toHaveBeenCalledTimes(1);
expect(logError).toHaveBeenCalledWith(`Failed to load core theme CSS from ${coreConfig.themeCore.urls.default}`);
expect(document.querySelector('link').href).toBe(`${getConfig().BASE_URL}/${PARAGON_THEME.paragon.themeUrls.core.fileName}`);
expect(initialState).toBeFalsy();
});

it('should not create any core link if is properly configured', () => {
Copy link
Member

Choose a reason for hiding this comment

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

[nit/clarification] What does "properly configured" mean here to make it not create the core link? Might be good to make the test case name a bit more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe like should not create any core link if can not find urls definition understanding this in the code

   // If there is no config for the core theme url, do nothing.
    if (!themeCore?.urls) {

themeOnLoad.mockImplementation(() => { initialState = true; });
const coreConfig = {
themeCore: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/core.min.css',
},
onLoad: themeOnLoad,
};
renderHook(() => useParagonThemeCore(coreConfig));
expect(document.head.querySelectorAll('link').length).toBe(0);
expect(themeOnLoad).toHaveBeenCalledTimes(1);
expect(initialState).toBeTruthy();
});
});
159 changes: 159 additions & 0 deletions src/react/hooks/paragon/useParagonThemeVariants.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import { act, renderHook } from '@testing-library/react-hooks';
import { getConfig } from '../../../config';
import { logError } from '../../../logging';
import useParagonThemeVariants from './useParagonThemeVariants';

jest.mock('../../../logging');

const mockAddEventListener = jest.fn();
const mockRemoveEventListener = jest.fn();
const mockOnChange = jest.fn();

Object.defineProperty(window, 'matchMedia', {
value: jest.fn(() => ({
addEventListener: mockAddEventListener,
removeEventListener: mockRemoveEventListener,
onchange: mockOnChange,
})),
});

describe('useParagonThemeVariants', () => {
const themeOnLoad = jest.fn();
let initialState = false;

afterEach(() => {
document.head.innerHTML = '';
themeOnLoad.mockClear();
initialState = false;
logError.mockClear();
mockAddEventListener.mockClear();
mockRemoveEventListener.mockClear();
});

it('should create the links tags for each theme variant and change the state to true when are loaded', () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: "when are loaded" -> "when all variants are loaded" or similar

themeOnLoad.mockImplementation(() => { initialState = true; });
const themeVariants = {
light: {
urls: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/light.min.css',
brandOverride: 'https://cdn.jsdelivr.net/npm/@edx/brand@$brandVersion/dist/light.min.css',

Copy link
Member

Choose a reason for hiding this comment

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

nit: extra blank line

},
},
dark: {
urls: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/dark.min.css',
brandOverride: 'https://cdn.jsdelivr.net/npm/@edx/brand@$brandVersion/dist/dark.min.css',

Copy link
Member

Choose a reason for hiding this comment

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

nit: extra blank line

},
},
};
const currentThemeVariant = 'light';
renderHook(() => useParagonThemeVariants({ themeVariants, currentThemeVariant, onLoad: themeOnLoad }));
const themeLinks = document.head.querySelectorAll('link');
act(() => { themeLinks.forEach((link) => link.onload()); });

expect(themeLinks.length).toBe(4);
expect(initialState).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

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

Similar to other comment above, would recommend asserting that themeOnLoad was called, in place of the side of effect of what themeOnLoad mock is set up to do (i.e., mutate the initialState).

});

it('should dispatch a log error and fallback to PARAGON_THEME if can not load the core theme link', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Does the test name need to be updated, since we're testing the theme variant links, not the core theme link?

global.PARAGON_THEME = {
paragon: {
version: '1.0.0',
themeUrls: {
core: {
fileName: 'core.min.css',
},
variants: {
light: {
fileName: 'light.min.css',
default: true,
dark: false,
Copy link
Member

Choose a reason for hiding this comment

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

See other related commented. The config schema now has a defaults property that accepts a light and dark property instead of including default and dark in the variants object itself.

},
},
},
},
};
themeOnLoad.mockImplementation(() => { initialState = true; });
const themeVariants = {
light: {
urls: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/light.min.css',
},
},
};
const currentThemeVariant = 'light';
renderHook(() => useParagonThemeVariants({ themeVariants, currentThemeVariant, onLoad: themeOnLoad }));
const createdLinkTag = document.head.querySelector('link');
act(() => { createdLinkTag.onerror(); });
expect(logError).toHaveBeenCalledTimes(1);
expect(logError).toHaveBeenCalledWith(`Failed to load theme variant (${currentThemeVariant}) CSS from ${themeVariants.light.urls.default}`);
expect(document.querySelector('link').href).toBe(`${getConfig().BASE_URL}/${PARAGON_THEME.paragon.themeUrls.variants.light.fileName}`);
expect(initialState).toBeFalsy();
});

it('should configure themes acording with system preference and add the change event listener', () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: typo "acording" -> "according"

nit: "configure themes" -> "configure theme variants"

themeOnLoad.mockImplementation(() => { initialState = true; });
window.matchMedia['prefers-color-scheme'] = 'dark';

const themeVariants = {
light: {
urls: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/light.min.css',
brandOverride: 'https://cdn.jsdelivr.net/npm/@edx/brand@$brandVersion/dist/light.min.css',

Copy link
Member

Choose a reason for hiding this comment

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

nit: extra blank line

},
},
dark: {
urls: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/dark.min.css',
brandOverride: 'https://cdn.jsdelivr.net/npm/@edx/brand@$brandVersion/dist/dark.min.css',

Copy link
Member

Choose a reason for hiding this comment

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

nit: extra blank line

},
},
};

const currentThemeVariant = 'light';
renderHook(() => useParagonThemeVariants({
themeVariants,
currentThemeVariant,
onLoad: themeOnLoad,
}));

const themeLinks = document.head.querySelectorAll('link');
act(() => {
themeLinks.forEach((link) => link.onload());
window.matchMedia.matches = false;
});

expect(mockAddEventListener).toHaveBeenCalledTimes(1);
expect(initialState).toBeTruthy();
});

it('should do nothing if themeVariants is not configured', () => {
themeOnLoad.mockImplementation(() => { initialState = true; });
const themeVariants = null;
const currentTheme = 'light';
renderHook(() => useParagonThemeVariants({ themeVariants, currentTheme, onLoad: themeOnLoad }));
expect(document.head.querySelectorAll('link').length).toBe(0);
expect(initialState).toBeFalsy();
});

it('should do nothing if themeVariants is not configured properly', () => {
themeOnLoad.mockImplementation(() => { initialState = true; });
const themeVariants = {
light: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/light.min.css',
},
dark: {
default: 'https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/dark.min.css',
},
};

const currentTheme = 'light';
renderHook(() => useParagonThemeVariants({ themeVariants, currentTheme, onLoad: themeOnLoad }));
expect(document.head.querySelectorAll('link').length).toBe(0);
expect(initialState).toBeTruthy();
});
});