-
Notifications
You must be signed in to change notification settings - Fork 632
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
Fix Vue Tesing Library routes bug #12105
Fix Vue Tesing Library routes bug #12105
Conversation
Build Artifacts
|
jest.conf.js
Outdated
@@ -12,4 +12,6 @@ module.exports = Object.assign(baseConfig, { | |||
'packages/hashi/src/*.js', | |||
'packages/kolibri-tools/lib/src/*.js', | |||
], | |||
maxConcurrency: 2, |
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.
What is the reason for this? If the tests are having issues being parallelized, that tends to indicate there's an issue in the tests themselves (this only affects running on developer machines, as in a CI environment, Jest automatically runs tests sequentially instead).
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.
Noooo I thought I deleted it, Im sorry. I changed it bc it was taking all my processors and my computer died trying to run the tests.
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.
No worries - I did wonder if that was the case!
In future, you can also just pass additional jest CLI options: https://jestjs.io/docs/cli#--maxconcurrencynum for maxConcurrency and maxWorkers to the test invocation in Kolibri, it should pass through the additional arguments.
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.
Yes, for some reason I thought CLI options wasn't working but I think it was because I was passing maxConcurrency instead of maxWorkers. Thanks!
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.
Good to know, I had to do a bit of work to make sure our thin wrapper around jest passed through the additional CLI arguments, so I am glad that's still working!
940b23b
to
3e30d2c
Compare
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.
Looks good, and tests should be passing!
jest.conf.js
Outdated
@@ -12,4 +12,6 @@ module.exports = Object.assign(baseConfig, { | |||
'packages/hashi/src/*.js', | |||
'packages/kolibri-tools/lib/src/*.js', | |||
], | |||
maxConcurrency: 2, |
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.
No worries - I did wonder if that was the case!
In future, you can also just pass additional jest CLI options: https://jestjs.io/docs/cli#--maxconcurrencynum for maxConcurrency and maxWorkers to the test invocation in Kolibri, it should pass through the additional arguments.
Thank you @rtibbles! |
Summary
For now there is a bug when we use the render function of Vue testing library without passing a
routes
prop, this has forced us to import VueRouter, and pass aroutes: new VueRouter()
prop just to avoid this error, although we don't use it. This means that we have more boilerplate code even for the simplest tests.This error raises because for some reason vue router in its inner implementation checks if the
$option.router
object is defined, but just checking forundefined
values, so when this value is null, it gives a null pointer exception.Reviewer guidance
Check that VTL tests run successfully even if we dont pass the
routes
prop.Testing checklist
PR process
Reviewer checklist
yarn
andpip
)