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

Add visual regression testing library + tests #762

Closed
wants to merge 5 commits into from

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Jul 15, 2023

Description

Adds cypress-image-snapshot testing library into the repo to use for visual regression testing. This is based on jest-image-snapshot. This is particularly useful for charts since there is nothing baked in cypress to do that. In addition, charts rendered with vega are limited in that they are a single element and datapoints can't be examined via html.

The added helper function validateVisSnapshot() can be used by downstream plugins to persist and compare their own snapshots.

Adds tests for the Vis Augmenter feature in both OSD + AD plugin test suites as well.

Issues Resolved

Dashboards PR #4515

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ohltyler ohltyler marked this pull request as draft July 15, 2023 00:22
@ohltyler ohltyler marked this pull request as ready for review July 18, 2023 19:38
@ohltyler
Copy link
Member Author

CI fails to run 3.0 cluster, posting local results below (2.9 cluster):
OSD tests:

       Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✔  core-opensearch-dashboards/opensear      02:13        2        2        -        -        - │
  │    ch-dashboards/apps/vis-augmenter/da                                                         │
  │    shboard_spec.js                                                                             │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✔  All specs passed!                        02:13        2        2        -        -        -  

AD tests:

       Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✔  plugins/anomaly-detection-dashboard      05:55        5        5        -        -        - │
  │    s-plugin/vis_augmenter/associate_de                                                         │
  │    tector_spec.js                                                                              │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✔  All specs passed!                        05:55        5        5        -        -        -  

@SuZhou-Joe
Copy link
Member

SuZhou-Joe commented Jul 24, 2023

The cypress version this repo used does not comply with the peerDependencies of cypress-image-snapshot, do we have any other package that we can use?

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

I agree with @SuZhou-Joe, the library you picked hasnt been maintained in a while. There are a few other alternative's including one thats forked this to work with the version of Cypress that we are using. That being said, i dont think that just because the peer dependency isnt met that this will not work, as clearly seen by the PR, but in future, should something break, its likely not going to be fixed.

@@ -320,3 +320,35 @@ export const filterByObjectType = (type) => {
.click({ force: true });
cy.wait(3000);
Copy link
Member

Choose a reason for hiding this comment

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

not related to this PR but you guys have a lot of wait(time in ms) statements all across your tests. This is almost always unnecessary and can save you guys a lot of time in your tests.

Copy link
Member

Choose a reason for hiding this comment

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

I ran this test without installing the AD plugin on my instance and ran into a size mismatch error. That being said I thinks its probably because i didnt have the plugin installed, but just something to keep an eye out for.

@ohltyler
Copy link
Member Author

ohltyler commented Jul 26, 2023

It's interesting, I had originally picked that one since it was the most popular open source option I was seeing on Cypress' official page. But I also this forked one that is continuing to maintain it. I will try switching to this library. Thanks for the catch on the old cypress version dependency.

@ohltyler
Copy link
Member Author

ohltyler commented Jul 27, 2023

@SuZhou-Joe @ashwin-pc looking closer, all tags of the maintained cypress-image-snapshot require cypress to be >10.0.0. I see we have been locked at 9.5.4 since >1 year ago. Is there potential for bumping the cypress version here? I'm unsure what impact or effort there is regarding test environment setup in the release workflows and if this is a problem.

I can also still install with --legacy-peer-deps and sanity test that things work as expected, and if so, lock the version of that library so there is no new unexpected changes. But I don't think that's the best approach.

cc @seraphjiang @peterzhuamazon

@SuZhou-Joe
Copy link
Member

SuZhou-Joe commented Jul 28, 2023

image

From the screenshot of our CI flow detail, the final version we used seems to be v12.17.2, @ashwin-pc @peterzhuamazon do you know the original intension of this part? And I think we may be able to upgrade our cypress version to v12 for further maintenance as 1. we are actually using v12 now and 2. other cypress-plugins only support cypress >= v10.

@kavilla
Copy link
Member

kavilla commented Apr 2, 2024

@ohltyler I will close this PR as I haven't seen any updates. Please re-open if there was pending changes on the reviewers side or if you made changes. Or if there any follow up questions.

@kavilla kavilla closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants