refactor: prepare tests for migration #10509

maxpatiiuk · 2024-10-09T04:13:36Z

Make minor adjustments to Calcite code to have post-migration tests pass.

Changes that need to be done post-migration are encoded in the following codemod files in the arcgis-web-components repository:

/packages/support-packages/stencil-to-lit/src/code/fileSpecific.ts
/packages/support-packages/stencil-to-lit/src/component/fileSpecific.ts

At this point, I am finishing up the failing tests in the S components and will be pretty much done with all A-S component (with exception of #10394 and #10495)

maxpatiiuk · 2024-10-09T04:15:01Z

packages/calcite-components/src/components/color-picker/color-picker.e2e.ts

-      // initialize page with calcite-color-picker to make it available in the evaluate callback below
      const page = await newE2EPage({
-        html: "<calcite-color-picker></calcite-color-picker>",
+        html: "",
      });
-      await page.setContent("");


This seems wrong.
The html option of newE2EPage just calls setContent behind the scenes.
Each time setContent is called, the page is re-created from scratch - there is no information carried over in between.

TL;DR: it seems this workaround is no longer needed and we can simplify newProgrammaticE2EPage.

It looks like Stencil fixed an issue where creating an empty page and then adding a component via createElement would not load properly (i.e., the html content required a component). This was our workaround (basis for newProgrammaticE2EPage).

maxpatiiuk · 2024-10-09T04:15:09Z

packages/calcite-components/src/components/color-picker/color-picker.e2e.ts


      await page.evaluate(async (color) => {
        const picker = document.createElement("calcite-color-picker");
        picker.value = color;
        document.body.append(picker);

-        await new Promise<void>((resolve) => requestAnimationFrame(() => resolve()));
+        await picker.componentOnReady();


componentOnReady is more suited for this

maxpatiiuk · 2024-10-09T04:16:15Z

packages/calcite-components/src/components/loader/loader.tsx

@@ -104,7 +104,6 @@ export class Loader implements LocalizedComponent {
                [CSS.loaderPart]: true,
                [CSS.loaderPartId(index)]: true,
              }}
-              key={index}


key={index} seems useless?

Lumina has a warning about using index as a key (it suggests not using the key at all in those cases)

On closer inspection, I think key isn't needed here since the indices won't vary between renders.

maxpatiiuk · 2024-10-09T04:16:47Z

packages/calcite-components/src/components/navigation-user/navigation-user.e2e.ts

@@ -22,7 +22,7 @@ describe("calcite-navigation-user", () => {
      },
      {
        propertyName: "textDisabled",
-        value: "",
+        value: true,


In Stencil, casting is done both to property and attribute (thus setting boolean property to "" will cast to true). In Lit, casting is only done when converting attribute to property or property to attribute.

See https://qawebgis.esri.com/arcgis-components/?path=/docs/references-lumina-transition-from-stencil--docs#attribute-to-property-casting

maxpatiiuk · 2024-10-09T04:16:51Z

packages/calcite-components/src/components/navigation/navigation.e2e.ts

@@ -15,7 +15,7 @@ describe("calcite-navigation", () => {
    reflects("calcite-navigation", [
      {
        propertyName: "navigationAction",
-        value: "",
+        value: true,


maxpatiiuk · 2024-10-09T04:16:56Z

packages/calcite-components/src/components/pick-list-item/pick-list-item.e2e.ts

@@ -9,7 +9,7 @@ describe("calcite-pick-list-item", () => {
  });

  describe("honors hidden attribute", () => {
-    hidden("calcite-list-item");
+    hidden("calcite-pick-list-item");


Good catch!

maxpatiiuk · 2024-10-09T04:17:10Z

packages/calcite-components/src/components/rating/rating.e2e.ts

@@ -399,7 +399,7 @@ describe("calcite-rating", () => {
      expect(labels[4]).not.toHaveClass("partial");
    });

-    it("should update the ui of the rating when a hover event triggers on a rating label", async () => {
+    it("should update the UI of the rating when a hover event triggers on a rating label", async () => {


consistency with usages in previous test titles

maxpatiiuk · 2024-10-09T04:17:54Z

packages/calcite-components/src/components/shell-panel/shell-panel.e2e.ts

@@ -118,7 +118,7 @@ describe("calcite-shell-panel", () => {
      "calcite-shell-panel",
      (panel: HTMLCalciteShellPanelElement, containerClass: string, contentClass: string) => {
        const container = panel.shadowRoot.querySelector(containerClass);
-        return container.firstElementChild.className == contentClass;
+        return container.firstElementChild.classList.contains(contentClass);


In Lumina, when class={} prop is an object, the attribute is set to not just "value" but to " value " (notice the spaces.
Adjusting this to use classList.contains to have the code work both pre and post migration

maxpatiiuk · 2024-10-09T04:29:03Z

packages/calcite-components/src/utils/form.tsx

@@ -290,7 +290,7 @@ function invalidHandler(event: Event) {
        formComponent.status = "idle";
      }

-      if ("validationIcon" in formComponent && !formComponent.validationIcon) {
+      if ("validationIcon" in formComponent) {


Unless I am wrong, it seems like this condition should not have been here and it worked in Stencil accidentally:

Since validationIcon prop is of mixed type (string | boolean), setting it to empty string does not automatically cast to true

You set validationIcon prop to true here on form validation error:

calcite-design-system/packages/calcite-components/src/utils/form.tsx

Line 281 in fed772f

icon: true,

SInce this prop is reflected, stencil reflects it to empty string attribute:

https://github.com/ionic-team/stencil/blob/61f90b04ecf8896d74840ea295f949f3de38676c/src/runtime/vdom/set-accessor.ts#L178

Reflection of the attribute triggers attributeChangedCallback. Stencil sees that the attribute value "" does not match the property value true, and sets the property to "".

When it comes to clear the validation message, this place does the && !formComponent.validationIcon check. That worked because "" is falsy.

However, this condition does not work in Lit because Lit ignores attributeChangedCallback when it does the reflection of a property to an attribute

IIRC, this logic preserves user-set validation icons between invalid form submissions. You can see the behavior in this pen.

@benelan could you confirm? If so, could you look at updating formAssociated to cover this case?

IIRC, this logic preserves user-set validation icons between invalid form submissions.

Yeah this is correct. I definitely abused some insane javascript quirks here so that's my bad lol. If you need help refactoring this so it will work in Lit I can take another look.

If so, could you look at updating formAssociated to cover this case?

Yeah definitely.

Thanks for the fix, @benelan!

maxpatiiuk · 2024-10-10T04:03:37Z

packages/calcite-components/src/components/time-picker/time-picker.e2e.ts

@@ -1214,7 +1214,7 @@ describe("calcite-time-picker", () => {
      await page.mouse.click(buttonUpLocationX, buttonUpLocationY);
      await page.waitForChanges();
      const fractionalSecondEl = await page.find(`calcite-time-picker >>> .input.fractionalSecond`);
-      expect(fractionalSecondEl.innerHTML).toEqual("0");
+      expect(fractionalSecondEl.textContent).toEqual("0");


Lit inserts comments in the rendered content to help it deal with cases when nodes are inserted before/after lit-rendered nodes by third-party code.
To avoid those comments from appearing in this test, use textContent instead of innerHTML

Good tip. We should probably add a warning against innerHTML usage.

github-actions · 2024-10-28T02:01:09Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

maxpatiiuk · 2024-10-28T15:33:58Z

Ping.
Looks like #10509 (comment) is the only remaining thing blocking this PR?

**Related Issue:** #10482 ## Summary Resolve the `validationIcon` issue brought up in #10509 (comment) The only properties affected by the change in how boolean properties are reflected are `icon`, `validationIcon`, and `download`. This PR fixes the utils that caused issues with `icon` and `validationIcon`. The `download` property seems to already be correct, for example: https://github.com/Esri/calcite-design-system/blob/25319998739c3af17a5438d285c5931ea656285d/packages/calcite-components/src/components/link/link.tsx#L178

maxpatiiuk · 2024-11-01T01:56:24Z

@benelan thanks for the fix!

With #10662 merged, #10509 (comment) is resolved, and this PR is unblocked.

jcfranco

Awesome! Thanks, @maxpatiiuk!

maxpatiiuk requested a review from jcfranco October 9, 2024 04:13

maxpatiiuk self-assigned this Oct 9, 2024

github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Oct 9, 2024

maxpatiiuk commented Oct 9, 2024

View reviewed changes

maxpatiiuk force-pushed the max/tests-migration branch from fed772f to cda7494 Compare October 10, 2024 04:02

maxpatiiuk commented Oct 10, 2024

View reviewed changes

jcfranco mentioned this pull request Oct 25, 2024

refactor: simplify newProgrammaticE2EPage #10613

Merged

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Oct 28, 2024

This was referenced Oct 31, 2024

refactor: handle stencil vs lumina boolean reflection differences #10661

Closed

refactor: handle stencil vs lumina boolean reflection differences #10662

Merged

refactor: prepare tests for migration

d65c81b

maxpatiiuk force-pushed the max/tests-migration branch from cda7494 to d65c81b Compare November 1, 2024 01:55

maxpatiiuk requested review from jcfranco and removed request for jcfranco November 1, 2024 01:56

github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Nov 1, 2024

This comment was marked as duplicate.

Sign in to view

jcfranco approved these changes Nov 1, 2024

View reviewed changes

jcfranco added the skip visual snapshots Pull requests that do not need visual regression testing. label Nov 1, 2024

jcfranco merged commit 30367fd into dev Nov 1, 2024
15 checks passed

jcfranco deleted the max/tests-migration branch November 1, 2024 16:10

github-actions bot added this to the 2024-11-19 - 3.0.0 Nov Breaking Change Milestone milestone Nov 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: prepare tests for migration #10509

refactor: prepare tests for migration #10509

maxpatiiuk commented Oct 9, 2024 •

edited

Loading

maxpatiiuk Oct 9, 2024

jcfranco Oct 13, 2024

maxpatiiuk Oct 9, 2024

maxpatiiuk Oct 9, 2024

jcfranco Oct 13, 2024

maxpatiiuk Oct 9, 2024

maxpatiiuk Oct 9, 2024

maxpatiiuk Oct 9, 2024

jcfranco Oct 12, 2024

maxpatiiuk Oct 9, 2024

maxpatiiuk Oct 9, 2024

maxpatiiuk Oct 9, 2024

jcfranco Oct 13, 2024

benelan Oct 18, 2024

jcfranco Nov 1, 2024

maxpatiiuk Oct 10, 2024

jcfranco Oct 12, 2024

github-actions bot commented Oct 28, 2024

maxpatiiuk commented Oct 28, 2024

maxpatiiuk commented Nov 1, 2024

This comment was marked as duplicate.

jcfranco left a comment

refactor: prepare tests for migration #10509

refactor: prepare tests for migration #10509

Conversation

maxpatiiuk commented Oct 9, 2024 • 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

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

github-actions bot commented Oct 28, 2024

maxpatiiuk commented Oct 28, 2024

maxpatiiuk commented Nov 1, 2024

This comment was marked as duplicate.

jcfranco left a comment

Choose a reason for hiding this comment

maxpatiiuk commented Oct 9, 2024 •

edited

Loading