-
Notifications
You must be signed in to change notification settings - Fork 1
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
Configure vitest with API and Pinia mocking #229
Conversation
Fixes the issue in vue test utils where $q needs the dark property.
web/src/api/index.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are to export Enums. export type
only exports type
declarations and Enums are technically not types. TIL.
@@ -20,5 +20,5 @@ const pinia = createPinia(); | |||
const app = createApp(App); | |||
|
|||
app.use(pinia); | |||
app.use(Quasar, {}); | |||
app.use(Quasar, { plugins: { Dark } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin had to be enabled to avoid an error. I couldn't find a way around this, or what caused it. Perhaps using the dark
prop uses the Dark plugin behind the scenes. This wasn't documented, but components would not mount using Vue Test Utils without enabling this since $q.dark
's properties were accessed.
"src/**/*.d.ts", | ||
"src/**/*.tsx", | ||
"src/**/*.vue", | ||
"tests/**/*.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to add our test files to ensure they got our TypeScript settings and the path alias.
@@ -1,3 +1,5 @@ | |||
/// <reference types="vitest" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed at the top to expand the config type to include test
to set the Vitest testing environment to jsdom
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock response data and store data here is very large. My suggestion would be to, in future work, break these out into another file to quickly import with a helpful name.
vi.mock('src/api'); | ||
vi.mocked(useApi).mockReturnValue(api); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit was extremely tricky. vi.mock('src/api')
with no manual factory
set will:
Vitest will mock the module itself by invoking it and mocking every export.
The following principles apply
- All arrays will be emptied
- All primitives and collections will stay the same
- All objects will be deeply cloned
- All instances of classes and their prototypes will be deeply cloned
So great we get a nice mock with no effort. However our API module has two exports that rely on each other:
export const api = new PublishingClientApi();
export const useApi = () => api;
and Vitest has no clue about this when it constructs its mock.
So api
correctly gets setup as a new, mocked PublishingClientApi
but useApi
gets mocked as a vi.fn
with no return.
vi.mocked(useApi).mockReturnValue(api);
Is here to recreate that relationship so both api
can be mocked here, and in components useApi()
will return the mocked PublishingClientApi
class instance.
This can probbably be broken out into a __mock__
file at some point to avoid the boilerplate, but its so small that that felt like a future task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a funny call when separating out the test data. I think it would be useful to have a way of having base test data which could be pulled in from an external file, but then also overlay specific test data changes on top of it, for a particular test. This can obviously be created organically as we go forward, but it would be good to outline a small pattern to use so we could all be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree. Having test data in an external file that we can override partially would be perfect. I think another PR introducing that pattern with another test would be a great next step.
|
||
describe('description', () => { | ||
test('some test description', async() => { | ||
vi.mocked(api.files.get, { partial: true }).mockResolvedValue({ data: fakeResponse }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a bit tricky. Once the mocking was fixed this became pretty easy! The partial: true
allows us to avoid needing to fully type the AxiosResponse
object instead only setting the thing(s) we need (data
in this case).
|
||
const wrapper = mount(FilesToPublish, { | ||
global: { | ||
plugins: [Quasar, createTestingPinia({ createSpy: vi.fn })] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each time we mount we need to enable the Quasar
plugin, and create a testing Pinia instance (if we need Pinia for the test(s)). Since we don't use globals we need to create a spy ourselves which is very straightforward.
}, | ||
pythonRequirements: ['YW55aW89PTMuNi4yCmFzZ2lyZWY9PTMuNi4wCmNsaWNrPT04LjEuMwpmYXN0YXBpPT0wLjk1LjIKaDExPT0wLjE0LjAKaWRuYT09My40CnB5ZGFudGljPT0xLjEwLjcKcHlqd3Q9PTIuNy4wCnJzY29ubmVjdC1weXRob249PTEuMTcuMApzZW12ZXI9PTIuMTMuMApzaXg9PTEuMTYuMApzbmlmZmlvPT0xLjMuMApzdGFybGV0dGU9PTAuMjcuMAp0eXBpbmctZXh0ZW5zaW9ucz09NC41LjAKdXZpY29ybj09MC4yMi4wCg=='] | ||
}; | ||
await flushPromises(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also extremely tricky. Without this our tests execute before the component has the opportunity to re-render after the deploymentStore.deployment
is set.
So we need to mount the component to get the Pinia store, but we need to wait until that is all done before we move on. One way to do this is with flushPromises
which will resolve all promise handles and ensure async operations and DOM updates happen before our expects.
The docs for Vue Test Utils also recommend using nextTick
to wait for the DOM, but this doesn't work. I assume this is because we are waiting for some async code in Pinia, and not ONLY the Vue tick to occur for updating.
References:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks great! Comments added only for clarification and ideas for the future. Thanks!
} | ||
}); | ||
|
||
const deploymentStore = useDeploymentStore(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit strange to me that we call this after we mount the component. I would think of this as part of the test setup, all available ahead of the component being mounted (at which time it would run through all of its normal behavior).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The Pinia docs don't explicitly say it, but in their examples always have the use store code under the mount 1. If we move it above we get an error:
FAIL tests/unit/components/configurePublish/FilesToPublish.test.ts > description > some test description
Error: [🍍]: "getActivePinia()" was called but there was no active Pinia. Did you forget to install pinia?
const pinia = createPinia()
app.use(pinia)
This will fail in production.
Since the mount is actually creating the Pinia we need to do it first. I actually had the same reaction and tried moving it above when things weren't quite working wondering if that was a fix.
Footnotes
vi.mock('src/api'); | ||
vi.mocked(useApi).mockReturnValue(api); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a funny call when separating out the test data. I think it would be useful to have a way of having base test data which could be pulled in from an external file, but then also overlay specific test data changes on top of it, for a particular test. This can obviously be created organically as we go forward, but it would be good to outline a small pattern to use so we could all be consistent.
}); | ||
|
||
const deploymentStore = useDeploymentStore(); | ||
deploymentStore.deployment = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the deployment store being so "mono" across all of our settings, we're definitely going to want to put something better in place so we're not always copying/pasting the state over and over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree. I think it will look very similar to your proposal of importing things from an external file and overriding where necessary. In a follow-up PR introducing this will be important as soon as we have more than a single test.
Verified that the new unit tests were run and passed in CI. |
This PR creates a single test for
FilesToPublish.vue
to setup our patterns for unit testing our frontend with Vitest, Vue Test Utils, and Pinia's testing tooling.Dependencies added:
@vue/test-utils
for mounting components and easily testing themjsdom
to test in a web environment (and access web APIs) rather than being constrained by a Node testing environment@pinia/testing
to create a Pinia instance specifically for unit testingConfig changes:
jsdom
include
for the TypeScript config to get the path alias and settingsundefined
error.API Type Changes:
DeploymentFile
since our API currently doesn't return it, and our mocked returns need to match our typesexport types
fixed this and allowed us to import the Enums for use in the testing files.Intent
Addresses #167
Approach
This PR doesn't fully test
FilesToPublish.vue
instead it focuses on getting a single test with mocks working for easy expansion. This allows us to review the pattern / methodology used as opposed to a whole suite of tests as well.Dependencies and configurations were added while establishing a
FilesToPublish.test.ts
file. With each error encountered something was adjusted until a full test was created with the tooling we needed to expand.Since this is only one test I did not pull out util functions, mock data files, or anything else to reduce the size of the files or create anything to reduce duplication in new tests. That can be done when new tests are actually created.
Directions for Reviewers
npm run test:unit
in theweb
directory OR runjust web/test-unit
to runvitest
web
directory on its own since the Vitest extension doesn't handle the configs being nested well.