Verify that we've navigated to Settings by checking the title. #21245
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjcenizal
added
the
Feature:Visualizations
|
CC @elastic/kibana-qa |
|
This looks like it is related to #19743, but doesn't directly address it based on the discussion. |
💔 Build Failed |
💚 Build Succeeded |
|
Retest |
💚 Build Succeeded |
|
Retest |
💚 Build Succeeded |
|
Retest |
💚 Build Succeeded |
|
Retest |
💚 Build Succeeded |
|
Retest |
💚 Build Succeeded |
LeeDr
approved these changes
Jul 26, 2018
There was a problem hiding this comment.
LGTM - I didn't test this but Jenkins has multiple times and it looks like a pretty obvious improvement.
The built-in find timeout should wait a few seconds for the managementSettingsTitle to exist which should prevent errors if the next action tried to do something on that page. And if it fails, it will narrow down where the problem was sooner than without it.
| @@ -45,7 +45,7 @@ export function SettingsPageProvider({ getService, getPageObjects }) { | |||
|
|
|||
| async clickKibanaSettings() { | |||
| await this.clickLinkText('Advanced Settings'); | |||
| await PageObjects.header.waitUntilLoadingHasFinished(); | |||
| await testSubjects.exists('managementSettingsTitle'); | |||
There was a problem hiding this comment.
I think there is some confusion about what this function does. It returns true or false, it doesn't assert that that does indeed exist. I think the default time is like 1 second, so this is the equivalent of adding a 1 second sleep. :) If you want to assert that it does exist, use find instead, as that will retry for something like 60 seconds, and fail if it is never found.
The function should probably be renamed getExists instead of exists to eliminate that confusion.
There was a problem hiding this comment.
Good catch. It does return true or false. And it has a 1 second timeout, but I don't think that means it's like a 1 second sleep. If it does find it it will return as quickly as it can. If it doesn't find it within 1 second it will return false.
There was a problem hiding this comment.
Well, in the latter case, if it doesn't find it, it's like adding a 1 second sleep. In the former case, it's like adding a variable sleep, up to 1 seconds, possibly quicker. I'm wondering if that's why the tests appear more stable.
|
|
||
| // Verify navigation is successful, or else fail the test consuming this. | ||
| expect(isSettingsLoaded).to.be(true); | ||
| return isSettingsLoaded; |
There was a problem hiding this comment.
Why return the value here? Looks like it's not used anywhere, and the name of the function doesn't really indicate it.
There was a problem hiding this comment.
An async method expects a promise as a return value or if a promise isn't returned then it wraps the return value in Promise.resolve. I don't really know whether that means it's OK to return undefined or not, but resolving undefined feels unexpected to me. I don't have a strong feeling on this so if you feel like something else would make more sense, I'll roll with that.
There was a problem hiding this comment.
Resolve to undefined makes more sense to me then resolving to true, as it would take looking into the function to understand what the value of true indicated. It doesn't even really mean anything because it always returns true, or fails. When I think of a function resolving to true, I think it can also resolve to false. Not that the option is resolve(true) or reject(). Anyway, most clickX functions don't return anything, so for the sake of consistency, I think we should follow suit.
There was a problem hiding this comment.
Works for me, I'll change it. Thanks!
|
@stacey-gammon Done. |
stacey-gammon
approved these changes
Jul 26, 2018
|
@LeeDr @stacey-gammon I added a |
| @@ -37,6 +38,14 @@ export function TestSubjectsProvider({ getService }) { | |||
| return await find.existsByDisplayedByCssSelector(testSubjSelector(selector), timeout); | |||
| } | |||
|
|
|||
| async existOrFail(selector, timeout = 1000) { | |||
| log.debug(`TestSubjects.existOrFail(${selector})`); | |||
| const doesExist = await find.existsByDisplayedByCssSelector(testSubjSelector(selector), timeout); | |||
There was a problem hiding this comment.
why not just do const doesExist = await this.exists(selector, timeout);?
There was a problem hiding this comment.
Good point, fixed!
stacey-gammon
approved these changes
Jul 26, 2018
| async existOrFail(selector, timeout = 1000) { | ||
| log.debug(`TestSubjects.existOrFail(${selector})`); | ||
| const doesExist = await this.exists(selector, timeout); | ||
| // Verify element exists, or else fail the test consuming this. |
There was a problem hiding this comment.
nit: comment seems unnecessary. But up to you!
There was a problem hiding this comment.
I normally would agree with you but I think the intention behind most of this test code becomes easily obscured so I've been erring on the side of caution with my comments.
| @@ -53,6 +53,6 @@ export default function ({ getService, loadTestFile }) { | |||
| loadTestFile(require.resolve('./_histogram_request_start')); | |||
| loadTestFile(require.resolve('./_vega_chart')); | |||
| loadTestFile(require.resolve('./_lab_mode')); | |||
| loadTestFile(require.resolve('./_linked_saved_searches.js')); | |||
| // loadTestFile(require.resolve('./_linked_saved_searches.js')); | |||
There was a problem hiding this comment.
Oh don't forget to uncomment this
| @@ -37,6 +38,13 @@ export function TestSubjectsProvider({ getService }) { | |||
| return await find.existsByDisplayedByCssSelector(testSubjSelector(selector), timeout); | |||
| } | |||
|
|
|||
| async existOrFail(selector, timeout = 1000) { | |||
There was a problem hiding this comment.
I kind of like the name expectExists. But up to you.
💔 Build Failed |
💚 Build Succeeded |
|
Retest |
💚 Build Succeeded |
|
Retest |
💔 Build Failed |
LeeDr
approved these changes
Jul 26, 2018
|
Just got some unrelated failures. I believe these are the same ones @LeeDr saw. |
|
Retest |
💚 Build Succeeded |
|
Retest |
💚 Build Succeeded |
cjcenizal
added a commit
to cjcenizal/kibana
that referenced
this pull request
Jul 27, 2018
…ic#21245) * Fail the consuming test if navigation to settings is unsuccessful. * Add testSubjects.existOrFail helper.
cjcenizal
added a commit
that referenced
this pull request
Jul 27, 2018
… (#21321) * Fail the consuming test if navigation to settings is unsuccessful. * Add testSubjects.existOrFail helper.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses flaky
afterAll:We should make this the canonical way of verifying navigation. Users look for titles to verify their location, and so should our functional tests.