test: add testing to useParagonTheme hooks #514
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @dcoa! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## ags/inject-theme-css #514 +/- ##
=========================================================
+ Coverage 68.61% 79.05% +10.43%
=========================================================
Files 48 48
Lines 1351 1351
Branches 283 283
=========================================================
+ Hits 927 1068 +141
+ Misses 360 251 -109
+ Partials 64 32 -32 ☔ View full report in Codecov by Sentry. |
dcoa
changed the title
dcoa
changed the title
dcoa
force-pushed
the
ags/inject-theme-css
branch
2 times, most recently
from
333318b
to
34b5184
Compare
dcoa
force-pushed
the
ags/inject-theme-css
branch
from
34b5184
to
82ac477
Compare
There was a problem hiding this comment.
Thanks for the testing contribution, @dcoa! 🙌 Left some initial feedback.
| 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', |
There was a problem hiding this comment.
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.
| themeOnLoad.mockClear(); | ||
| logError.mockClear(); |
There was a problem hiding this comment.
nit: could use jest.clearAllMocks()
| act(() => createdLinkTag.onload()); | ||
| expect(createdLinkTag.href).toBe(coreConfig.themeCore.urls.default); | ||
| expect(themeOnLoad).toHaveBeenCalledTimes(1); | ||
| expect(initialState).toBeTruthy(); |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
I agree, due to is a parameter and not a hook method shouldn't test its functionality (out of the scope).
| default: true, | ||
| dark: false, |
There was a problem hiding this comment.
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',
},
},
},
},
};| act(() => { themeLinks.forEach((link) => link.onload()); }); | ||
|
|
||
| expect(themeLinks.length).toBe(4); | ||
| expect(initialState).toBeTruthy(); |
There was a problem hiding this comment.
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).
| expect(initialState).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should dispatch a log error and fallback to PARAGON_THEME if can not load the core theme link', () => { |
There was a problem hiding this comment.
Does the test name need to be updated, since we're testing the theme variant links, not the core theme link?
| mockRemoveEventListener.mockClear(); | ||
| }); | ||
|
|
||
| it('should create the links tags for each theme variant and change the state to true when are loaded', () => { |
There was a problem hiding this comment.
nit: "when are loaded" -> "when all variants are loaded" or similar
| expect(initialState).toBeFalsy(); | ||
| }); | ||
|
|
||
| it('should configure themes acording with system preference and add the change event listener', () => { |
There was a problem hiding this comment.
nit: typo "acording" -> "according"
nit: "configure themes" -> "configure theme variants"
| 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', | ||
|
|
| 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', | ||
|
|
|
Thanks for the updates, @dcoa! Your changes LGTM; I will go ahead and merge into the base PR's branch now and see where we're at with any additional coverage required. |
adamstankiewicz
merged commit e96de6b
into
openedx:ags/inject-theme-css
|
@dcoa 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
chore: update package-lock.json chore: update package-lock.json take 2 chore: remove console.log statements fix: ignore system preference change when theme variant set in localstorage chore: add tests for updates to AppProvider chore: update react-intl to pass peer dependencies after pinning all deps chore: split hooks.js up into separate files and begin some related tests test: add testing to useParagonTheme hooks (openedx#514) * test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
chore: update package-lock.json chore: update package-lock.json take 2 chore: remove console.log statements fix: ignore system preference change when theme variant set in localstorage chore: add tests for updates to AppProvider chore: update react-intl to pass peer dependencies after pinning all deps chore: split hooks.js up into separate files and begin some related tests test: add testing to useParagonTheme hooks (openedx#514) * test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
chore: update package-lock.json chore: update package-lock.json take 2 chore: remove console.log statements fix: ignore system preference change when theme variant set in localstorage chore: add tests for updates to AppProvider chore: update react-intl to pass peer dependencies after pinning all deps chore: split hooks.js up into separate files and begin some related tests test: add testing to useParagonTheme hooks (openedx#514) * test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
chore: update package-lock.json chore: update package-lock.json take 2 chore: remove console.log statements fix: ignore system preference change when theme variant set in localstorage chore: add tests for updates to AppProvider chore: update react-intl to pass peer dependencies after pinning all deps chore: split hooks.js up into separate files and begin some related tests test: add testing to useParagonTheme hooks (openedx#514) * test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
Description:
This is a testing contribution to Theming Configuration in Runtime.
Context: #440
C.C @adamstankiewicz