Add ability to whitelist visible elements in percy #45733

nickofthyme · 2019-09-13T22:50:59Z

Summary

Add the ability to whitelist OR blacklist visible elements in percy visual regression tests.

blacklist: select elements to ignore/hide default
whitelist: select only elements you want to show in the screenshot

Add SnapshotOptions to snapshot method in visual regression service.

interface SnapshotOptions {
    name?: string;
    add?: string[];
    remove?: string[];
}

The selectors options accept an array on testSubject selectors to identify which elements to hide or show.

Note when using the show option. You will not be able to hide children of the show elements.

See example Percy diff here. Notice the first whitelists only the chart and the second blacklists the side panel

@liza-mae are you ok with me un-skipping the discover tests to see if they are stable with these changes?

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-09-14T00:30:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

nickofthyme · 2019-09-16T15:49:57Z

@spalger could you take a look at this when you get a minute? Thanks

cchaos · 2019-09-16T16:35:39Z

I think we should consider the risks this pose to the value of Percy. Visual testing is definitely important to purely visual components like graphs. However, it's also vital to ensure that the PR doesn't contain changes that affect the surrounding UI. By enabling white listing, you can more easily completely ignore what effects your changes have on the surrounding UI.

Take your screenshots for example, it not only ignores the outer UI, but the container the component sits in. Therefore, you'll miss the fact that the new graph's x-axis is actually extending beyond the container's bounds:

Another example is say, in your graph you wanted the text red. So you added .text { color: red } but didn't realize that the sidebar also has a .text selector and now all of a sudden all the text in the sidebar is red as well.

This won't be picked up by Percy because the test completely isolates only the component you've deemed necessary.

The nice thing about Percy, is that while it will indicate that there are differences, you can always accept them. It's a more manual process, but at the very least makes you consider your changes and how it affects other areas.

I am of course using the general "you" for all devs.

nickofthyme · 2019-09-16T20:03:33Z

@cchaos Yup I agree entirely. I think of this as merely a temporary fix as we venture into the world of visual testing and work through all of the kinks.

TLDR;
This approach is an additive approach to build up to a more fully captured visual testing suite.

The issue we are facing right now is that the Percy tests are currently unstable. The maps tests are removing everything but the canvas element from the page to avoid erroneous or minor diffs.

The discover page is facing similar issues on top of the general issue of Percy not completely loading assets, such as font-awesome, before taking the screenshot.

I think the approach of whitelisting will help us build up to a final standard of the visual testing, while in the meantime not skipping tests because the current implementation needs to be fine-tuned. Also given we are charged per screenshot per PR per push, it would be ideal to test as much as we can in one shot as you have pointed out.

You pointed out another issue related to canvas elements which is that the chart is shifted in the full discovery page. This is again an implementation issue as the canvas elements are swapped with imgs then the screenshot is taken. If the browser widths are different, it skews the layout around canvas elements. We need to refine this approach and come up with a stable way of doing this.

spalger · 2019-09-16T21:03:29Z

@nickofthyme these failing checks aren't run on draft PRs, if you want to start with a draft in the future. Otherwise please resolve them?

test/visual_regression/tests/discover/chart_visualization.js

test/visual_regression/services/visual_testing/visual_testing.ts

cchaos · 2019-09-17T12:50:01Z

Gotcha, thank you for the background. My concerns aren't meant to hinder progress for sure. I mainly wanted to raise them to consider why we're using Percy in the first place. But I see there's still a ways to go before full utilization, like fixing the font issue.

elasticmachine · 2019-09-18T00:30:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

This looks great, but I just saw this article and now I'm wondering if we could accomplish the same functionality with a call signature like:

takePercySnapshot({ show: ['foo'], hide: ['bar'] })

The idea would be that when show.length > 0 the .percyHideByDefault class would be added to the body, hide selectors would always be applied.

nickofthyme · 2019-09-18T14:20:20Z

@spalger yeah I saw you mention deciding to whitelist or blacklist. I'll take a look at the article.

test/visual_regression/services/visual_testing/take_percy_snapshot.js

src/legacy/core_plugins/kibana/public/index.scss

elasticmachine · 2019-09-19T18:04:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-20T23:32:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Changes for elastic#45733

test/visual_regression/tests/discover/chart_visualization.js

elasticmachine · 2019-09-21T02:18:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-21T23:20:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

nickofthyme · 2019-09-23T13:52:05Z

@spalger Are you ok with me merging this?

elasticmachine · 2019-09-23T15:32:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

test/visual_regression/tests/discover/chart_visualization.js

elasticmachine · 2019-09-25T01:05:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

LGTM

elasticmachine · 2019-09-27T15:24:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

nickofthyme · 2019-09-27T15:59:21Z

@elastic/kibana-design could someone take a look at this before merging, please? Thanks!

cchaos

Removal from SASS looks fine

* add ability to whitelist visible elements in percy * allow white and blacklisting elements in visual tests * remove unnecessary webElement methods * refactor snapshot options to use show and hide * refactor add/remove and visual test helpers * [percy] rework css rules to allow hiding inside shown elements * [percy] adjust logic to support showing inside hiding * attach styles to hide percy when capturing the snapshot * refactor in order to make sure all logic is executed if snapshot fails * remove sleeps * add back skipFirefox tag

add ability to whitelist visible elements in percy

d4e1db0

nickofthyme added the Feature:Visual Testing label Sep 13, 2019

nickofthyme requested review from spalger and liza-mae September 13, 2019 22:50

nickofthyme requested a review from a team as a code owner September 13, 2019 22:50

spalger reviewed Sep 16, 2019

View reviewed changes

test/visual_regression/tests/discover/chart_visualization.js Outdated Show resolved Hide resolved

test/visual_regression/services/visual_testing/visual_testing.ts Outdated Show resolved Hide resolved

nickofthyme added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 labels Sep 16, 2019

spalger added the v7.5.0 label Sep 16, 2019

nickofthyme added 3 commits September 17, 2019 14:40

Merge branch 'master' into fix/percy-flackyness

2617c72

allow white and blacklisting elements in visual tests

0e44af7

remove unnecessary webElement methods

53a38ff

spalger reviewed Sep 18, 2019

View reviewed changes

refactor snapshot options to use show and hide

a57bb4b

spalger reviewed Sep 19, 2019

View reviewed changes

nickofthyme added 2 commits September 20, 2019 16:46

refactor add/remove and visual test helpers

e5c5df9

Merge branch 'master' into fix/percy-flackyness

32ab1cb

spalger added 3 commits September 20, 2019 17:13

[percy] rework css rules to allow hiding inside shown elements

34df258

[percy] adjust logic to support showing inside hiding

d3c686a

attach styles to hide percy when capturing the snapshot

0d8b078

spalger and others added 2 commits September 20, 2019 17:18

refactor in order to make sure all logic is executed if snapshot fails

c970305

Merge pull request #1 from spalger/pr/45733

2903ad0

Changes for elastic#45733

spalger reviewed Sep 21, 2019

View reviewed changes

test/visual_regression/tests/discover/chart_visualization.js Outdated Show resolved Hide resolved

remove sleeps

c767047

Merge branch 'master' into fix/percy-flackyness

45674d0

spalger suggested changes Sep 23, 2019

View reviewed changes

test/visual_regression/tests/discover/chart_visualization.js Outdated Show resolved Hide resolved

nickofthyme added 2 commits September 24, 2019 18:09

add back skipFirefox tag

ff34f65

Merge branch 'master' into fix/percy-flackyness

d729958

spalger approved these changes Sep 26, 2019

View reviewed changes

Merge branch 'master' into fix/percy-flackyness

118cd60

cchaos approved these changes Sep 27, 2019

View reviewed changes

nickofthyme merged commit 858ad4d into elastic:master Sep 27, 2019

nickofthyme deleted the fix/percy-flackyness branch September 27, 2019 17:07

liza-mae mentioned this pull request Oct 31, 2019

[7.x] Add ability to whitelist visible elements in percy (#45733) #49880

Merged

liza-mae mentioned this pull request Oct 31, 2019

[7.5] Add ability to whitelist visible elements in percy (#45733) #49881

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to whitelist visible elements in percy #45733

Add ability to whitelist visible elements in percy #45733

nickofthyme commented Sep 13, 2019 •

edited

Loading

elasticmachine commented Sep 14, 2019

nickofthyme commented Sep 16, 2019

cchaos commented Sep 16, 2019

nickofthyme commented Sep 16, 2019 •

edited

Loading

spalger commented Sep 16, 2019

cchaos commented Sep 17, 2019

elasticmachine commented Sep 18, 2019

spalger left a comment •

edited

Loading

nickofthyme commented Sep 18, 2019

elasticmachine commented Sep 19, 2019

elasticmachine commented Sep 20, 2019

elasticmachine commented Sep 21, 2019

elasticmachine commented Sep 21, 2019

nickofthyme commented Sep 23, 2019

elasticmachine commented Sep 23, 2019

elasticmachine commented Sep 25, 2019

spalger left a comment

elasticmachine commented Sep 27, 2019

nickofthyme commented Sep 27, 2019 •

edited

Loading

cchaos left a comment

Add ability to whitelist visible elements in percy #45733

Add ability to whitelist visible elements in percy #45733

Conversation

nickofthyme commented Sep 13, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Sep 14, 2019

💚 Build Succeeded

nickofthyme commented Sep 16, 2019

cchaos commented Sep 16, 2019

nickofthyme commented Sep 16, 2019 • edited Loading

spalger commented Sep 16, 2019

cchaos commented Sep 17, 2019

elasticmachine commented Sep 18, 2019

💚 Build Succeeded

spalger left a comment • edited Loading

Choose a reason for hiding this comment

nickofthyme commented Sep 18, 2019

elasticmachine commented Sep 19, 2019

💔 Build Failed

elasticmachine commented Sep 20, 2019

💚 Build Succeeded

elasticmachine commented Sep 21, 2019

💚 Build Succeeded

elasticmachine commented Sep 21, 2019

💚 Build Succeeded

nickofthyme commented Sep 23, 2019

elasticmachine commented Sep 23, 2019

💚 Build Succeeded

elasticmachine commented Sep 25, 2019

💚 Build Succeeded

spalger left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 27, 2019

💚 Build Succeeded

nickofthyme commented Sep 27, 2019 • edited Loading

cchaos left a comment

Choose a reason for hiding this comment

nickofthyme commented Sep 13, 2019 •

edited

Loading

nickofthyme commented Sep 16, 2019 •

edited

Loading

spalger left a comment •

edited

Loading

nickofthyme commented Sep 27, 2019 •

edited

Loading