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: Migrate jest to use projects feature #322
Conversation
// This method is internal to the engine and should only be used for testing purposes! | ||
// The component registry need to get flushed between each test to avoid having the | ||
// engine warning about multiple class registration with the same tag name. | ||
export function flushComponentRegistry() { |
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.
@davidturissini Here is the newly added method that we discussed on last Friday.
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.
let's chat about this. We should discuss if we really need a registry for now or not. Also, if this method is supposed to stay, it should throw if it is ever invoked outside of test mode. Let's add the if condition here.
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.
Let's remove the registry.
So, we were using this feature a while back (projects) and it was very buggy. Has this become more stable? I recall there being many caching issues with this. |
I didn't had any issue. I would be interested if you can pull this branch and give a try at it. |
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
851eb6f
to
7e7d3ac
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
scripts/jest/root.config.js
Outdated
@@ -0,0 +1,16 @@ | |||
const path = require('path'); |
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.
Please remove
7e7d3ac
to
bfe25cf
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
@@ -76,7 +76,7 @@ describe('error boundary component', () => { | |||
return html; | |||
} | |||
} | |||
const boundaryHostElm = createElement('x-boundary', {is: BoundryHost}); | |||
const boundaryHostElm = createElement('x-parent', {is: BoundryHost}); |
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.
why are you changing this?
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 changed it not to conflict with line 70, one of the children of x-boundary
is also a x-boundary
but with a different class. The engine logs a warning because of this.
@@ -7,27 +7,28 @@ import { VNode } from "../../3rdparty/snabbdom/types"; | |||
import { Component } from "../component"; | |||
import { unwrap } from "../main"; | |||
import { querySelector } from "../dom"; | |||
import { callbackify } from "util"; |
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.
hate when we have util package, it messes up with our regular autoimports if they match the same exported name :(
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.
Nice catch. I will remove this, it was inserted by VSCode!
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.
let's chat about this.
… into pmdartus/jest-restructure
Benchmark resultsBase commit: lwc-engine-benchmark
|
@@ -85,7 +84,8 @@ const hook: Hooks = { | |||
renderVM(vm); | |||
}, | |||
create(oldVNode: VNode, vnode: VNode) { | |||
createVM(vnode.sel as string, vnode.elm as HTMLElement, vnode.data.slotset); | |||
const { slotset, ctor } = vnode.data; |
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 branch seems to be out of sync with master.
const Ctor = getCtorByTagName(sel); | ||
const { forceTagName } = Ctor as ComponentConstructor; | ||
|
||
const Ctor = options.is; |
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.
again, this is very out of sync with master! Here is you have to untangle Ctor, and them get the forceTagName from it.
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.
you need to rebase master, which is very different from what you have in this PR
Benchmark resultsBase commit: lwc-engine-benchmark
|
Rebased with the latest master. |
@@ -91,25 +90,26 @@ export function createElement(sel: string, options: any = {}): HTMLElement { | |||
if (!isObject(options) || isNull(options)) { | |||
throw new TypeError(); | |||
} | |||
const { is } = (options as any); | |||
|
|||
const { is: Ctor } = (options as any); |
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.
You need to do the circular check here too, otherwise you can't extract the tagName. In the old code, check that we first register it, then we extract it from the registry (which is already resolved if it is circular)
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.
Small issue for circular dependencies on root elements with forceTagName defined. I don't think we have tests for this, but this will fail for it.
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.
@caridy I updated the PR to handle __circular__
refs. I move the logic in utils.ts
and refactor all the usage in the codebase to use this helper method.
@@ -91,7 +91,9 @@ export function createElement(sel: string, options: any = {}): HTMLElement { | |||
throw new TypeError(); | |||
} | |||
|
|||
const { is: Ctor } = (options as any); | |||
let { is: Ctor } = (options as any); | |||
Ctor = resolveCircularModuleDependency(Ctor); |
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.
nip: single liner: const Ctor = resolveCircularModuleDependency(options.is)
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.
LGTM, small nip.
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Details
This PR changes the way jest is configured in the mono-repo, to leverage the
projects
feature properly. This would allow us to have a different jest config for each project.Changes:
root
instead oftemplate
In upcoming a PR I want to get rid of all the remaining warnings by introducing a custom matcher that will trap the warnings and the errors (#94, #104). All the unexpected console
log
,warn
anderror
should make the test fail. Doing this would greatly improve quality and the consistency of the warnings the engine surfaces.Does this PR introduce a breaking change?