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

Fix flakey chromatic visual regression tests #10038

Closed
sophschneider opened this issue Aug 11, 2023 · 5 comments
Closed

Fix flakey chromatic visual regression tests #10038

sophschneider opened this issue Aug 11, 2023 · 5 comments
Assignees

Comments

@sophschneider
Copy link
Contributor

sophschneider commented Aug 11, 2023

For some reason, stories with Popover and/or Tooltip are indeterminately changing every time chromatic regression tests are run. Examples:

We should figure out why this is happening and fix it

@jesstelford
Copy link
Contributor

A couple hypotheses:

  1. Caused by layout shift
    • Chromatic is taking the screenshot, then layout shift is occurring.
    • This could happen due to timeouts in the rendering pipeline forcing Chromatic to take the screenshot before the entire component has finished rendering. Eg; the VMs running the browser is under higher than normal load, so rendering takes longer, etc.
  2. Caused by inconsistent JS positional calculations
    • The Popover relies on performing JS-based layout calculations after initial render/mount.
    • These calculations could be taking variable amounts of time, so the screenshot is sometimes taken before, and sometimes taken after the full positional calculation is completed.
    • Alternatively, it could be the results of the calculations themselves being inconsistent. Perhaps due to timing with mounting and other browser layout tasks (see Remove .vscode folder #1 above), or something to do with the order of rendering the components in the story.
  3. Caused by async font loading
    • We load the Google font CSS in a blocking way before stories are rendered.
    • But that CSS triggers a non-blocking/async load of the Inter font from Google's CDN (browsers can decide the priority of loading fonts, and it defaults to "low" afaict)
    • So the screenshot might sometimes be taken when the font has fully loaded and rendered, then other times be taken when the font hasn't yet loaded (which would affect component layout and sizing, etc)
    • Possible fix here: [Storybook] Fix font-loading in Chromatic #9993

@sophschneider
Copy link
Contributor Author

sophschneider commented Aug 14, 2023

I was able to replicate a layout shift in storybook:

  1. go to a flakey story (anything with an open Tooltip or Popover, e.g. LegacyCard with separate header
  2. Empty cache and hard reload
  3. remount component on story to see layout shift

Interestingly, subsequent refreshes don't cause a layout shift, only the first after an empty cache

replicate-shift.mov

@sophschneider
Copy link
Contributor Author

sophschneider commented Aug 14, 2023

seems to be flakey calculations in PositionedOverlay

with the same window size and process followed from above

Before remount After remount
Image Image

As you can see, the left px value is changing/calculating differently

@sophschneider
Copy link
Contributor Author

It looks like the activatorRect is the value changing. When I console log the bounding box for the activator when replicating the issue, I get changing x, left, and width values

Image

@sophschneider
Copy link
Contributor Author

I've determined this is a font loading issue.

Since the width is being incorrectly calculated, I put a debugger after the first bounding box calculation. At that point, the font is smaller than the final render causing a different bounding box calculation

First bounding box calculation Correct bounding box calculation
Image Image

I tried pulling #9993 but it didn't fix the issue

@sophschneider sophschneider self-assigned this Aug 14, 2023
sophschneider added a commit that referenced this issue Aug 15, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
sophschneider added a commit that referenced this issue Aug 15, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
@sophschneider sophschneider reopened this Aug 15, 2023
@sophschneider sophschneider removed their assignment Aug 15, 2023
sam-b-rose pushed a commit that referenced this issue Aug 15, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
@sophschneider sophschneider self-assigned this Aug 15, 2023
sam-b-rose pushed a commit that referenced this issue Aug 15, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
sam-b-rose pushed a commit that referenced this issue Aug 15, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
sam-b-rose pushed a commit that referenced this issue Aug 15, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
sam-b-rose pushed a commit that referenced this issue Aug 15, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
kyledurand pushed a commit that referenced this issue Aug 16, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
sophschneider added a commit that referenced this issue Aug 17, 2023
### WHY are these changes introduced?

Fixes #10038

Use system font in chromatic snapshots (use Inter still in storybook) so
that we can use the regression tests without flakiness during our se23
consolidation work.

I made an [issue here](#10104)
to figure out how to get stories to not load until the font has loaded
so we can use the proper font.

### WHAT is this pull request doing?

Using the [`isChromatic`](https://www.chromatic.com/docs/ischromatic)
function to only control behaviour in snapshots, this removes the font
loading link from the document preview head.
 
### How to 🎩

* Check the [UI Tests
CI](https://shopify.chromatic.com/build?appId=5d559397bae39100201eedc1&number=19503)
to see the system font updated in all snapshots
* Check that the published [storybook in
CI](https://5d559397bae39100201eedc1-tyfbdvqiif.chromatic.com/) is still
using Inter
kyledurand pushed a commit that referenced this issue Aug 17, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
kyledurand pushed a commit that referenced this issue Aug 17, 2023
### WHY are these changes introduced?

Fixes #10038

Use system font in chromatic snapshots (use Inter still in storybook) so
that we can use the regression tests without flakiness during our se23
consolidation work.

I made an [issue here](#10104)
to figure out how to get stories to not load until the font has loaded
so we can use the proper font.

### WHAT is this pull request doing?

Using the [`isChromatic`](https://www.chromatic.com/docs/ischromatic)
function to only control behaviour in snapshots, this removes the font
loading link from the document preview head.
 
### How to 🎩

* Check the [UI Tests
CI](https://shopify.chromatic.com/build?appId=5d559397bae39100201eedc1&number=19503)
to see the system font updated in all snapshots
* Check that the published [storybook in
CI](https://5d559397bae39100201eedc1-tyfbdvqiif.chromatic.com/) is still
using Inter
sophschneider added a commit that referenced this issue Sep 19, 2023
### WHY are these changes introduced?

Resolves #10038

### WHAT is this pull request doing?

* Preloads the `inter` font to avoid flakey width/postion calculations
in chromatic snapshots (more context in the issue)
* Run chromatic in CI when `polaris-react/.storybook` changes
* Accept chromatic baseline UI snapshots using inter

> Note: I think inter now shows when the beta flag is off as well but
this is ok as we are on the `next` branch and in the process of removing
the beta flag

### How to 🎩

#### To duplicate the original bug:
1. Go to [this
story](https://storybook.polaris.shopify.com/?path=/story/all-components-legacycard--with-separate-header&globals=polarisSummerEditions2023:true)
2. Make sure the cache is empty (empty cache and hard reload)
3. Refresh (or rerender) to see popover overlay shift

<details>
<summary>Replicate Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/53ed5e19-7c8d-4ee4-ab95-81a2c08e279d
</details>

#### To tophat fix:
1. Go to [this story on this PR's
chromatic](https://5d559397bae39100201eedc1-qhkfhdyjuo.chromatic.com/?path=/story/all-components-legacycard--with-separate-header)
2. Repeat steps 2-3 above

<details>
<summary>Fixed Bug Demo</summary>


https://github.com/Shopify/polaris/assets/20652326/de28ff59-84ab-46bd-b28a-31dbede8f001
</details>
sophschneider added a commit that referenced this issue Sep 19, 2023
### WHY are these changes introduced?

Fixes #10038

Use system font in chromatic snapshots (use Inter still in storybook) so
that we can use the regression tests without flakiness during our se23
consolidation work.

I made an [issue here](#10104)
to figure out how to get stories to not load until the font has loaded
so we can use the proper font.

### WHAT is this pull request doing?

Using the [`isChromatic`](https://www.chromatic.com/docs/ischromatic)
function to only control behaviour in snapshots, this removes the font
loading link from the document preview head.
 
### How to 🎩

* Check the [UI Tests
CI](https://shopify.chromatic.com/build?appId=5d559397bae39100201eedc1&number=19503)
to see the system font updated in all snapshots
* Check that the published [storybook in
CI](https://5d559397bae39100201eedc1-tyfbdvqiif.chromatic.com/) is still
using Inter
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

No branches or pull requests

2 participants