Tests for visualization screenshot comparison #17545
Conversation
💔 Build Failed |
bhavyarm
force-pushed
the
addScreenShotPieCompare
branch
from
b922b0f
to
ba3fb0f
Compare
💔 Build Failed |
💔 Build Failed |
bhavyarm
force-pushed
the
addScreenShotPieCompare
branch
from
80981c8
to
b90ebdc
Compare
💔 Build Failed |
|
Jenkins, test this |
💔 Build Failed |
bhavyarm
force-pushed
the
addScreenShotPieCompare
branch
from
b90ebdc
to
0dc35a9
Compare
bhavyarm
changed the title
jbudz
added
test
Feature:Visualizations
💔 Build Failed |
|
You have a baseline/screenshot_tag_cloud.png which isn't used in the test? Maybe it was replaced by screenshot_tagcloud_single.png? |
|
My first run of this PR had rather bad results; Through some trial and error and looking at the diff screenshots I found that if I set the window size in the visualize/index.js page to 1291 x 811 Based on this, I'd like us to try to devise a screen size tuning routine that takes some screenshot and adjusts the Kibana window to get the smallest diff. This could even allow the tests to pass across multiple browsers. |
|
Updated the screenshot for data table options and trying again to see if CI goes through. |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
|
@bhavyarm On my Ubuntu machine, if I set my setWindowSize(1290,811) then my screenshots are exactly the size of the baseline screenshots and I get very good matching. Some are less than .01 difference and the worst one was about 0.022. The question is, why do I have to adjust my setWindowSize different than when you did the baseline. Even in the last passing Jenkins job you can see that the new session screenshot size didn't match the baseline size. The height is 9 pixels taller on the session. |
|
I'm going to try this PR on Windows and see what kind of sizes and diffs I get. |
|
@LeeDr see if this works when you get a moment. Thanks! |
💚 Build Succeeded |
|
On my Windows 4k 15" laptop with display scaling set to 100% and |
|
The latest changes with the calibration routine passes on my Ubuntu desktop and my Windows laptop on multiple difference display scaling settings. |
💔 Build Failed |
|
Jenkins, test this |
…rest of the screenshots
bhavyarm
force-pushed
the
addScreenShotPieCompare
branch
from
55787c6
to
115e72b
Compare
💔 Build Failed |
|
I feel like we need to have version control and branching on these images if they're going to fit into our standard processes. So to me, that means they go in some github elastic repository. When a Kibana PR makes a change that requires updating a screenshot, it should reference the PR or commit in the screenshots repo (and always update the screenshots version file?). Maybe there's an automated way to do this? Another approach is for each test to reference the specific screenshot blob url. In this way only individually changed screenshots would have to be refreshed when there was a change. We should consider solutions that make the PR process as easy as possible for both the case where a single screenshot needs to be updated or added, and the case where many or all of them have to be updated. |
|
|
@tylersmalley @cjcenizal @jbudz @silne30 - can you please add your inputs here? Lee and I will talk with Viz team in their next sync. Thanks! |
|
While I understand this adds much-needed coverage for Visualizations, I have a few additional concerns. The screenshot comparisons as implemented rely on a threshold, which in some cases allows for an 8% difference. With the subtleties of visualizations, I feel like this reduces the actual coverage. Understanding that the problem is the test coverage of visualizations, is there a reason we can't implement these tests in any of the current frameworks without adding an additional way of testing. Additionally, with the new visualization pipeline changes - will that help with the problem of test coverage? |
|
I had a run-in with visual regression tests while developing a test suite for eui and came into a few snags that seem to be inherent for that type of testing. Comes With The Territory
Compensating For Variances
|
|
@silne30 why did you merge this PR? |
|
I think it was an accident, but I'll let @silne30 explain that one. 😄 In terms of what to do next, should we revert this? If we do revert it and then decide we want to add this change, then we'd have to revert the revert. Would that re-add the images, thus ballooning the repo size further? |
This reverts commit 84d678b.
|
@tylersmalley Definitely wasn't intentional. I am not sure if there is some kind of default action on the page for enter keys or different key strokes but the only action I actually meant to take with this issue was to leave the comment that I left. I had no interaction with the merge issue button, whatsoever. Prior to me being requested to comment on this issue, I had no knowledge of it and it was not on my radar. Sorry causing the merge...however it was that it happened. |
* undoing a messy merge * updating screenshots * changing the variance to account for data table failure * trying a different variance for data table and a general one for the rest of the screenshots * changing the variance for general to .065 * adding xy position to adjust the screensize * changing variance and setting a small window * create calibrateForScreenshots method * remove empty lines
* undoing a messy merge * updating screenshots * changing the variance to account for data table failure * trying a different variance for data table and a general one for the rest of the screenshots * changing the variance for general to .065 * adding xy position to adjust the screensize * changing variance and setting a small window * create calibrateForScreenshots method * remove empty lines
elastic#19692) This reverts commit 84d678b.
This test adds baseline screenshots for all visualizations except tsvb and vega and compares them to ensure visualizations are getting displayed.