test: migrate test suites with skipped unit tests to karma #1753

ravijayaramappa · 2020-03-04T22:21:56Z

Details

Migrate following tests to karma suite

packages/@lwc/engine/src/framework/__tests__/html-element.spec.ts
remove unit tests related to global attribute restriction, restriction was removed via chore: remove attribute mutation restrictions #1362
packages/@lwc/engine/src/framework/__tests__/scoped-ids.spec.ts
Error message updated via refactor(engine): clean up error messages #1193
packages/@lwc/engine/src/__tests__/traverse.spec.ts
element's childNodes value will reflect default content. Updating test to follow spec.
Removed redundant tests that were testing lifecycle callbacks.
packages/@lwc/engine/src/__tests__/focus.spec.ts
Removed skipped tests that were meant to test non-existent functionality. Resurrect if we solve [refactor tests]: find ways to test isFocusable and isTabbable in a browser #1341

Does this PR introduce breaking changes?

✅ No, it does not introduce breaking changes.

If yes, please describe the impact and migration path for existing applications.

The PR fulfills these requirements:

Have tests for the proposed changes been added? ✅
Have you followed these instructions to clearly describe the issue being fixed or feature enhanced? ✅

GUS work item

W-7102290

salesforce-best-lwc-internal · 2020-03-05T01:24:28Z

⚠ Performance Regression

Best has detected that there is a 6.9% performance regression across your benchmarks.

Please click here to see more details.

Click to view significantly changed benchmarks

lwc-engine-benchmark

✅ Improvements	base (`fa28b98`)	target (`b018ae2`)	trend
wc-append-1k.benchmark/benchmark-table-wc/append/1k	490.45 (± 5.87ms)	464.17 (± 10.38ms)	-26.3ms (5.4%)

❌ Regressions	base (`fa28b98`)	target (`b018ae2`)	trend
table-clear-1k.benchmark/benchmark-table/clear/1k	11.55 (± 0.19ms)	12.66 (± 0.30ms)	+1.1ms (9.6%)
table-create-1k.benchmark/benchmark-table/create/1k	160.00 (± 1.51ms)	170.94 (± 2.15ms)	+10.9ms (6.8%)
tablecmp-clear-1k.benchmark/benchmark-table-component/clear/1k	5.36 (± 0.09ms)	6.57 (± 0.33ms)	+1.2ms (22.6%)
tablecmp-create-1k.benchmark/benchmark-table-component/create/1k	248.88 (± 2.21ms)	263.12 (± 2.86ms)	+14.2ms (5.7%)

ekashida

These tests rely on compileTemplate which we want to move away from (#1295). Could we move these tests to karma integration and enable them there?

restriction was removed via #1362

pmdartus · 2020-03-10T09:50:57Z

packages/integration-karma/test/component/decorators/api/index.spec.js

@@ -104,3 +105,9 @@ describe('inheritance', () => {
        expect(elm.overriddenMethod()).toBe('overridden - child');
    });
 });
+
+it('should not log an error when initializing api value to null', function() {


Suggested change

it('should not log an error when initializing api value to null', function() {

it('should not log an error when initializing api value to null', () => {

pmdartus · 2020-03-10T09:51:25Z

packages/integration-karma/test/component/decorators/track/index.spec.js

@@ -91,6 +93,12 @@ describe('object mutations', () => {
            expect(elm.shadowRoot.querySelector('.obj').textContent).toBe('1');
        });
    });
+
+    it('should not log an error when setting tracked value to null', function() {


Suggested change

it('should not log an error when setting tracked value to null', function() {

it('should not log an error when setting tracked value to null', () => {

pmdartus · 2020-03-10T09:52:19Z

packages/integration-karma/test/component/decorators/track/index.spec.js

@@ -124,3 +132,13 @@ describe('array mutations', () => {
        });
    });
 });
+
+describe('non-observable values', () => {
+    it('should not throw error when accessing a non-observable property from tracked property when not rendering', function() {


Suggested change

it('should not throw error when accessing a non-observable property from tracked property when not rendering', function() {

it('should not throw an error when accessing a non-observable property from a tracked property when not rendering', () => {

pmdartus · 2020-03-10T09:53:58Z

packages/integration-karma/test/component/lockerIntegration/LockerIntegration.spec.js

+    describe('with locker', () => {
+        it('should support manual construction', () => {
+            const elm = createElement('x-parent', { is: LockerIntegration });
+            document.body.appendChild(elm);


We should check in this case if the element is rendered properly, by checking the DOM output.

pmdartus · 2020-03-10T09:55:09Z

packages/integration-karma/test/shadow-dom/Element-properties/Element.assignedSlot.spec.js

+        expect(child.assignedSlot).toBe(null);
+    });
+
+    it('should return correct slot when native element is slotted', () => {


Suggested change

it('should return correct slot when native element is slotted', () => {

it('should return the correct slot when native element is slotted', () => {

pmdartus · 2020-03-10T09:56:17Z

packages/integration-karma/test/shadow-dom/Element-properties/Element.assignedSlot.spec.js

+        expect(child.assignedSlot).toBe(slot);
+    });
+
+    it('should return correct slot when custom element is slotted', () => {


Suggested change

it('should return correct slot when custom element is slotted', () => {

it('should return the correct slot when custom element is slotted', () => {

pmdartus · 2020-03-10T09:56:26Z

packages/integration-karma/test/shadow-dom/Element-properties/Element.assignedSlot.spec.js

+        expect(child.assignedSlot).toBe(null);
+    });
+
+    it('should return correct slot when text is slotted', () => {


Suggested change

it('should return correct slot when text is slotted', () => {

it('should return the correct slot when text is slotted', () => {

pmdartus · 2020-03-10T09:57:23Z

packages/integration-karma/test/shadow-dom/Element-properties/Element.assignedSlot.spec.js

@@ -0,0 +1,62 @@
+import { createElement } from 'lwc';


I would suggest adding a test for named slots as well.

pmdartus · 2020-03-10T09:59:53Z

packages/integration-karma/test/shadow-dom/HTMLElement-properties/CustomInstanceSetter.spec.js

+
+import CustomInstanceSetter from 'x/customInstanceSetter';
+
+describe('#data layer', () => {


The description doesn't reflect what the test is doing.

pmdartus · 2020-03-10T10:02:30Z

...gration-karma/test/shadow-dom/HTMLElement-properties/GlobalHTML.properties.accessKey.spec.js

@@ -0,0 +1,72 @@
+import { createElement } from 'lwc';


There is a lot of boilerplate code in the global HTML properties test. Is it something we can refactor to share the common test code?

ekashida · 2020-03-10T22:23:57Z

packages/integration-karma/helpers/test-utils.js

+        var filtered = [];
+        const childNodes = host.childNodes;
+        for (var i = 0; i < childNodes.length; i++) {
+            var n = childNodes[i];
+            if (!(n.nodeType === Node.COMMENT_NODE && n.tagName.startsWith('#shadow-root'))) {
+                filtered.push(n);
+            }
+        }
+        return filtered;


Nitpick:

Suggested change

var filtered = [];

const childNodes = host.childNodes;

for (var i = 0; i < childNodes.length; i++) {

var n = childNodes[i];

if (!(n.nodeType === Node.COMMENT_NODE && n.tagName.startsWith('#shadow-root'))) {

filtered.push(n);

}

}

return filtered;

return Array.from(host.childNodes).filter(n => !(

n.nodeType === Node.COMMENT_NODE && n.tagName.startsWith('#shadow-root')

));

Just saw that this used to be:

function getHostChildNodes(host) { return [...host.childNodes].filter(n => { return n.nodeType !== Node.COMMENT_NODE && !n.tagName.startsWith('#shadow-root'); }); }

This was incorrect because we only wanted to filter out #shadow-root comment nodes, right?

test-utils is not transpiled down to ES5, it is appended as-is. So will use something like this

return Array.prototype.slice.call(host.childNodes).filter(function(n) { return !( n.nodeType === Node.COMMENT_NODE && n.tagName.startsWith('#shadow-root')); });

This was incorrect because we only wanted to filter out #shadow-root comment nodes, right?

yes. It was failing if n was a text node.

ekashida · 2020-03-10T22:26:15Z

packages/integration-karma/helpers/test-utils.js

    return {
        registerForLoad: registerForLoad,
        clearRegister: clearRegister,
        load: load,
        extractDataIds: extractDataIds,
        extractShadowDataIds: extractShadowDataIds,
+        getHostChildNodes: getHostChildNodes,


Nit:

Suggested change

getHostChildNodes: getHostChildNodes,

getHostChildNodes,

Same as above, no es6 syntax.

ekashida · 2020-03-10T23:41:44Z

packages/integration-karma/test/component/decorators/track/index.spec.js

@@ -124,3 +132,13 @@ describe('array mutations', () => {
        });
    });
 });
+
+describe('non-observable values', () => {
+    it('should not throw an error when accessing a non-observable property from a tracked property when not rendering', () => {


This test seems weirdly specific, I wonder if it's to prevent a regression. Do you know if there is a related issue for it?

Suggested change

it('should not throw an error when accessing a non-observable property from a tracked property when not rendering', () => {

it('should not throw an error when accessing a non-observable property from a tracked property before render', () => {

Added back in 2017 and no linked issue a34c102#diff-048a3be16e6b62a0fcdb17e0b220b01f

ekashida · 2020-03-10T23:46:21Z

packages/integration-karma/test/component/lockerIntegration/LockerIntegration.spec.js

+
+import LockerIntegration from 'x/lockerIntegration';
+
+describe('integration', () => {


Nit: I think we should either collapse these two describe blocks into locker integration or remove them completely since our infrastructure implicitly uses the filename as the top level describe.

ekashida · 2020-03-11T00:05:45Z

packages/integration-karma/test/component/lockerIntegration/LockerIntegration.spec.js

+        it('should support manual construction', () => {
+            const elm = createElement('x-secure-parent', { is: LockerIntegration });
+            document.body.appendChild(elm);
+            // Verifying that shadow tree was created to ensure the component class was successfull processed


This comment suggests to me that if the component was not successfully processed the host element will be rendered without its shadow. Is this correct?

Suggested change

// Verifying that shadow tree was created to ensure the component class was successfull processed

// Verifying that shadow tree was created to ensure the component class was successfully processed

There are two ways this can fail.

createElement can fail to process the provided component class.

Once the host is connected to the dom, the engine can fail to run the lifecycle hooks of the component class. eg: render() in this testcase

ekashida · 2020-03-11T01:10:41Z

packages/integration-karma/test/shadow-dom/Element-properties/Element.id.spec.js

+                    /\[LWC error\]: Invalid id value "undefined". The id attribute must contain a non-empty string./
+                );
+                const child = elm.shadowRoot.querySelector('x-child');
+                expect(child.getAttribute('id')).toEqual(null);


Looks like this skipped test was incomplete. x-child doesn't have an id attribute so we need to add it. I tested it and saw that the value should be 'undefined'.

Updated the test. It seems like there is a difference in behavior for custom elements v/s standard html elements. When id value is undefined, standard html element return null.

Yeah, it has to do with how we set content vs IDL attribute for native vs custom elements.

Logged #1769 to document the difference. Added a comment in the test to link to the issue.

ekashida · 2020-03-11T01:12:05Z

packages/integration-karma/test/shadow-dom/Element-properties/Element.id.spec.js

+                    /\[LWC error\]: Invalid id value "". The id attribute must contain a non-empty string./
+                );
+                const child = elm.shadowRoot.querySelector('x-child');
+                expect(child.getAttribute('id')).toEqual(null);


Looks like this skipped test was incomplete. x-child doesn't have an id attribute so we need to add it. I tested it and saw that the value should be an empty string.

ekashida

Looks great, thanks Ravi!

pmdartus · 2020-03-11T12:26:57Z

Thanks for taking care of this @ravijayaramappa

* test: remove unit tests related to global attribute restriction restriction was removed via #1362 * test: update error message in scoped-ids.spec.ts * test: update test to match <slot> element spec * test: remove unit test that was testing non-existent functions * test: migrate tests to integration-karma * test: migrate scoped-ids.spec to karma suite * test: migrate html-element.spec.ts to karma * test: migrate traverse.spec.ts to karma suite (cherry picked from commit c1b56ec)

ravijayaramappa marked this pull request as ready for review March 5, 2020 20:24

ravijayaramappa requested a review from ekashida March 5, 2020 20:25

ekashida reviewed Mar 5, 2020

View reviewed changes

ravijayaramappa added 5 commits March 6, 2020 06:16

test: remove unit tests related to global attribute restriction

30c7956

restriction was removed via #1362

test: update error message in scoped-ids.spec.ts

8d34466

test: update test to match <slot> element spec

55300a5

test: remove unit test that was testing non-existent functions

883807c

test: migrate tests to integration-karma

18746fe

ravijayaramappa force-pushed the ravi/master/enable-unit-tests/W-7102290 branch from 8291e69 to 18746fe Compare March 6, 2020 15:36

ravijayaramappa changed the title ~~test: enable/remove skipped unit tests~~ test: migrate test suites with skipped unit tests to karma Mar 6, 2020

ravijayaramappa added the work-in-progress Work in progress label Mar 6, 2020

ravijayaramappa added 4 commits March 6, 2020 10:33

test: migrate scoped-ids.spec to karma suite

83b2954

test: migrate html-element.spec.ts to karma

daa229c

test: migrate traverse.spec.ts to karma suite

fe45739

test: karma report file not required

150df50

ravijayaramappa removed the work-in-progress Work in progress label Mar 7, 2020

pmdartus reviewed Mar 10, 2020

View reviewed changes

test: address pr feedback

54b1145

ekashida requested changes Mar 11, 2020

View reviewed changes

test: address pr comments round2

223dabb

ekashida approved these changes Mar 11, 2020

View reviewed changes

pmdartus approved these changes Mar 11, 2020

View reviewed changes

ravijayaramappa merged commit c1b56ec into master Mar 11, 2020

ravijayaramappa deleted the ravi/master/enable-unit-tests/W-7102290 branch March 11, 2020 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: migrate test suites with skipped unit tests to karma #1753

test: migrate test suites with skipped unit tests to karma #1753

ravijayaramappa commented Mar 4, 2020 •

edited

Loading

salesforce-best-lwc-internal bot commented Mar 5, 2020

lwc-engine-benchmark

ekashida left a comment

pmdartus Mar 10, 2020

pmdartus Mar 10, 2020

pmdartus Mar 10, 2020

pmdartus Mar 10, 2020

pmdartus Mar 10, 2020

pmdartus Mar 10, 2020

pmdartus Mar 10, 2020

pmdartus Mar 10, 2020

pmdartus Mar 10, 2020

pmdartus Mar 10, 2020

ekashida Mar 10, 2020

ekashida Mar 11, 2020 •

edited

Loading

ravijayaramappa Mar 11, 2020 •

edited

Loading

ravijayaramappa Mar 11, 2020 •

edited

Loading

ekashida Mar 10, 2020

ravijayaramappa Mar 11, 2020

ekashida Mar 10, 2020

ravijayaramappa Mar 11, 2020

ekashida Mar 10, 2020

ekashida Mar 11, 2020

ravijayaramappa Mar 11, 2020

ekashida Mar 11, 2020

ravijayaramappa Mar 11, 2020

ekashida Mar 11, 2020

ravijayaramappa Mar 11, 2020

ekashida Mar 11, 2020

ekashida left a comment

pmdartus commented Mar 11, 2020

	it('should not log an error when initializing api value to null', function() {
	it('should not log an error when initializing api value to null', () => {

	it('should not log an error when setting tracked value to null', function() {
	it('should not log an error when setting tracked value to null', () => {

	it('should not throw error when accessing a non-observable property from tracked property when not rendering', function() {
	it('should not throw an error when accessing a non-observable property from a tracked property when not rendering', () => {

	it('should return correct slot when native element is slotted', () => {
	it('should return the correct slot when native element is slotted', () => {

	it('should return correct slot when custom element is slotted', () => {
	it('should return the correct slot when custom element is slotted', () => {

	it('should return correct slot when text is slotted', () => {
	it('should return the correct slot when text is slotted', () => {


		import CustomInstanceSetter from 'x/customInstanceSetter';

		describe('#data layer', () => {


		import LockerIntegration from 'x/lockerIntegration';

		describe('integration', () => {

	// Verifying that shadow tree was created to ensure the component class was successfull processed
	// Verifying that shadow tree was created to ensure the component class was successfully processed

test: migrate test suites with skipped unit tests to karma #1753

test: migrate test suites with skipped unit tests to karma #1753

Conversation

ravijayaramappa commented Mar 4, 2020 • edited Loading

Details

Does this PR introduce breaking changes?

The PR fulfills these requirements:

GUS work item

salesforce-best-lwc-internal bot commented Mar 5, 2020

⚠ Performance Regression

lwc-engine-benchmark

ekashida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekashida Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

ravijayaramappa Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

ravijayaramappa Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekashida left a comment

Choose a reason for hiding this comment

pmdartus commented Mar 11, 2020

ravijayaramappa commented Mar 4, 2020 •

edited

Loading

ekashida Mar 11, 2020 •

edited

Loading

ravijayaramappa Mar 11, 2020 •

edited

Loading

ravijayaramappa Mar 11, 2020 •

edited

Loading