-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
unskip dashboard_screenshot tests by using new setScreenshotSize #46311
Conversation
💔 Build Failed |
💔 Build Failed |
@elasticmachine merge upstream |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
@LeeDr , I did some testing on Macbook 15" with different display scaling:
I think small resolutions may work better with an external display. I want to point out that I have seen tests fail pretty often with the following error in ES logs (causing dashboard panel being empty):
I was thinking it is caused by missing esArchiver unload, but was not able to prove it. |
💔 Build Failed |
💔 Build Failed |
@elasticmachine merge upstream |
💔 Build Failed |
await dashboardPanelActions.clickExpandPanelToggle(); | ||
await PageObjects.common.sleep(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the sleeps still needed in addition to the window resize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sleeps were part of my debugging along with those additional screenshots. See the issue about the skip line but it appears to be a Kibana bug. But it never failed for me locally and always fails on Jenkins. Oh, that reminds me I was going to try it on my Ubuntu 18 machine...
Since someone should eventually debug and fix whatever the problem is, I thought it was OK to leave the sleeps and additional screenshots in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I unskipped the second test and ran it on my Ubuntu thinking it might reproduce the failure we see on Jenkins, but it didn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: I did just reproduce it locally by running all the dashboard tests that come before this one. So something in a previous test is impacting this one. I'll work on figuring that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I think I fixed the test. The problem was that the dashboard tests generally don't use the navigate methods which put the timestamp in the URL, which means that this test was impacted by the browser cache from the previous test and the charts don't get the same colors (which, of course, makes the screenshot comparison fail).
I did leave one commented-out 1/2 second sleep in this test. We'll see if we have any failures on opening the context menu without it.
@elasticmachine merge upstream |
💚 Build Succeeded |
Jenkins retest |
💔 Build Failed |
Jenkins retest |
Jenkins please test this |
💔 Build Failed |
I know we all hate the idea of this, but I had to re-arrange the order of the test suites so that the one I'm unskipping is at the end. For the details why, see #46752 |
💚 Build Succeeded |
jenkins test this |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I left a comment about commented sleep
await PageObjects.dashboard.clickFullScreenMode(); | ||
// await PageObjects.common.sleep(500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LeeDr if the test will pass on CI, can we remove it?
retest |
💚 Build Succeeded |
Hopefully this won't be an issue
💚 Build Succeeded |
``` loadTestFile(require.resolve('./view_edit')); // Order of test suites *shouldn't* be important but there's a bug for the view_edit test above // elastic#46752 // The dashboard_snapshot test below requires the timestamped URL which breaks the view_edit test. // If we don't use the timestamp in the URL, the colors in the charts will be different. loadTestFile(require.resolve('./dashboard_snapshots')); ```
💚 Build Succeeded |
Summary
Added new
browser.setScreenshotSize(width, height)
method which figures the display scaling and browser boarders so it can setWindowSize to get the correct screenshot size.Note that depending on the display scaling on the machine running the test, the session screenshot may not be the same size as the baseline screenshot but will proportional so that the comparePng method will deliver good results.
Should make screenshot tests work in both headless and headed Chrome. I haven't tried any of this on Firefox or IE yet.
Fixes: #40173
Fixes: #19471
(the second one could have been already fixed by some other change but since the test was skipped we didn't know.)
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] This was checked for keyboard-only and screenreader accessibilityFor 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
We should not have to change
'notifications:lifetime:info': 10000
in config.js. I made this change because the notification for adding the first visualization to the dashboard was going away before the test could see it. It looks like it's going away before the default 5 seconds.