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

feat(test-utils): add lwc-test-utils with templateQuerySelector API #414

Merged
merged 9 commits into from
Jun 14, 2018

Conversation

trevor-bliss
Copy link
Contributor

@trevor-bliss trevor-bliss commented Jun 14, 2018

Details

Introduce lwc-test-utils that provides test helper functions to retrieve the ShadowRoot off of an LWCElement.

Depends on #417

Also update lwc-jest-preset to provide lwc-test-utils out of the box (as a hard dependency instead of a peerDependency!).

Does this PR introduce a breaking change?

  • Yes
  • No

If using lwc-jest-preset, users will need to upgrade to Jest >= 23.0.0 and can remove the lwc-jest-transformer, lwc-jest-resolver, and lwc-jest-serializer devDependencies from their project.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 91eb930 | Target commit: 08a804f

lwc-engine-benchmark

table-append-1k metric base(91eb930) target(08a804f) trend
benchmark-table/append/1k duration 142.30 (± 4.20 ms) 146.00 (± 5.00 ms) -2.60% 👌
table-clear-1k metric base(91eb930) target(08a804f) trend
benchmark-table/clear/1k duration 11.50 (± 0.60 ms) 11.70 (± 0.50 ms) -1.74% 👌
table-create-10k metric base(91eb930) target(08a804f) trend
benchmark-table/create/10k duration 839.00 (± 3.70 ms) 845.60 (± 4.20 ms) -0.79% 👎
table-create-1k metric base(91eb930) target(08a804f) trend
benchmark-table/create/1k duration 101.40 (± 1.80 ms) 101.40 (± 1.90 ms) 0.00% 👌
table-update-10th-1k metric base(91eb930) target(08a804f) trend
benchmark-table/update-10th/1k duration 85.00 (± 4.60 ms) 84.00 (± 3.60 ms) 1.18% 👌
tablecmp-append-1k metric base(91eb930) target(08a804f) trend
benchmark-table-component/append/1k duration 244.60 (± 5.00 ms) 246.60 (± 3.30 ms) -0.82% 👌
tablecmp-clear-1k metric base(91eb930) target(08a804f) trend
benchmark-table/clear/1k duration 35.20 (± 1.45 ms) 36.50 (± 1.70 ms) -3.69% 👎
tablecmp-create-10k metric base(91eb930) target(08a804f) trend
benchmark-table-component/create/10k duration 1678.90 (± 9.50 ms) 1703.40 (± 13.40 ms) -1.46% 👎
tablecmp-create-1k metric base(91eb930) target(08a804f) trend
benchmark-table-component/create/1k duration 194.80 (± 3.80 ms) 192.90 (± 3.90 ms) 0.98% 👌
tablecmp-update-10th-1k metric base(91eb930) target(08a804f) trend
benchmark-table-component/update-10th/1k duration 74.40 (± 3.80 ms) 77.30 (± 4.20 ms) -3.90% 👎

Copy link
Member

@ekashida ekashida left a comment

Choose a reason for hiding this comment

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

I'm not sure where in this set of proposed changes we would do so, but I think it would be great to:

  1. Mention the best practice of not digging too deep into the component tree in order to keep tests robust.
  2. Talk about the problem that these functions are solving (i.e., future-proofing tests from breaking once we introduce shadow DOM semantics).

* @returns {Element} The first decendent that matches the given selectors.
*/
export function templateQuerySelector(element, selectors) {
return Element.prototype.querySelector.call(element, selectors);
Copy link
Member

Choose a reason for hiding this comment

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

@caridy @davidturissini is there any way to guarantee that we return only the elements in the current template? Otherwise, these functions will not really future-proof users from the introduction of shadow DOM semantics.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might have to expose something. Lets chat about this.

@trevor-bliss
Copy link
Contributor Author

@ekashida Responding to your two points:

  1. I think the "Testing Recipes" of the main doc site would be a good place to put that info: https://internal.lwcjs.org/guide/testing-advanced.html

  2. I have this line in the jsdoc above each function: "Use this utility over Element.querySelector to maintain consistent behavior through LWC's evolving Shadow DOM API implementation.". Do you think we need to expand on that?

Copy link
Collaborator

@apapko apapko left a comment

Choose a reason for hiding this comment

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

One question: should we sanitize the input of both utility functions to ensure that an incoming element is in fact a custom element?

@@ -0,0 +1,58 @@
# lwc-test-utils

A collection of utility functions to assist in unit testing Lightning web components.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe LWC components or Lightning Web Component to stay consistent

Copy link
Contributor Author

@trevor-bliss trevor-bliss Jun 14, 2018

Choose a reason for hiding this comment

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

Ya the capitalization is a common point of confusion. It's less about consistency and more about intent. This is how the doc team explained it to me:

Lightning Web Components --> the "model"/framework used to develop the components
Lightning web components --> the individual components developed using the LWC framework

I did lowercase on purpose because these utils are to help unit test individual components.

export function getShadowRoot(element) {
if (!element || !element.$$ShadowRoot$$) {
const tagName = element && element.tagName && element.tagName.toLowerCase();
throw new Error(`Attempting to retrieve the ShadowRoot of '${tagName || element}' but no shadowRoot property found`);
Copy link
Member

@ekashida ekashida Jun 14, 2018

Choose a reason for hiding this comment

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

"ShadowRoot" should probably just be "shadow root"

* @returns {ShadowRoot} The shadowRoot property of the given element
*/
export function getShadowRoot(element) {
if (!element || !element.$$ShadowRoot$$) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should check for the existence of element.$$ShadowRoot$$ because not all elements have a shadow root.

Copy link
Member

Choose a reason for hiding this comment

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

Missed the part where we expect the parameter to be an LWC Element.

const tagName = element && element.tagName && element.tagName.toLowerCase();
throw new Error(`Attempting to retrieve the ShadowRoot of '${tagName || element}' but no shadowRoot property found`);
}
return element.$$ShadowRoot$$;
Copy link
Member

Choose a reason for hiding this comment

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

return element.$$ShadowRoot$$ || null; since element.shadowRoot evaluates to null for elements without an attached shadow root.

Copy link
Member

Choose a reason for hiding this comment

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

Missed the part where we expect the parameter to be an LWC Element.

@trevor-bliss trevor-bliss merged commit c07376c into master Jun 14, 2018
@trevor-bliss trevor-bliss deleted the tbliss/templateQuerySelector branch June 14, 2018 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants