-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[tests/functional] opt out of getting started on navigation #11881
[tests/functional] opt out of getting started on navigation #11881
Conversation
cc0ebf2
to
9b4e19b
Compare
return await testSubjects.exists('gettingStartedContainer'); | ||
} | ||
|
||
async outOut() { |
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.
Typo: should be optOut
.
9b4e19b
to
a7fae68
Compare
…f-getting-started-on-navigation
a7fae68
to
1b77840
Compare
Pulled your changes and tested it locally. It opts out of the getting started link on navigation. LGTM . |
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- pulled your changes and tested it locally.
Never mind, I suppose if I stay clear of |
Ruh roh, CI appears to be failing around the Console functional tests: https://kibana-ci.elastic.co/job/elastic+kibana+pull-request+multijob-selenium/4688/consoleFull
|
So, when you visit |
@@ -68,14 +69,14 @@ uiRoutes | |||
}); | |||
notify.error('Please create a new index pattern'); | |||
kbnUrl.change(CREATE_INDEX_PATTERN_ROUTE); | |||
return; | |||
return Promise.halt(); //prevent route resolve |
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.
It might be good to explain for the benefit of our future selves why this is necessary via a comment in this file somewhere. Since this is being done in a couple of places maybe we can extract a function here that changes the route and return a halted promise?
27a6773
to
9dc3498
Compare
@@ -68,14 +67,14 @@ uiRoutes | |||
}); | |||
notify.error('Please create a new index pattern'); | |||
kbnUrl.change(CREATE_INDEX_PATTERN_ROUTE); | |||
return; | |||
throw WAIT_FOR_URL_CHANGE_TOKEN; |
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.
@ycombinator I tried a different approach, throwing this token from within a route setup work function will halt the setup work, allowing it to wait for kbnUrl
to change. What do you think?
/cc @cjcenizal
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.
@spalger Do you think this throw should just become part of the kbnUrl.change
function? It seems to me the two would always want to travel together, no?
9dc3498
to
f88c940
Compare
src/ui/public/routes/routes.js
Outdated
@@ -2,9 +2,11 @@ import RouteManager from './route_manager'; | |||
import 'angular-route/angular-route'; | |||
import { uiModules } from 'ui/modules'; | |||
const defaultRouteManager = new RouteManager(); | |||
import { ABORT_SETUP_WORK_TOKEN } from './route_setup_manager'; |
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.
Did you mean this to be WAIT_FOR_URL_CHANGE_TOKEN
?
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.
Oops
|
||
// prevent moving forward, return a promise that never resolves | ||
// so that the $router can observe the $location update | ||
return Promise.halt(); |
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.
Super minor nit, but this seems more like the "early exit" exception, and throwing the error seems more like the default flow, so I'd suggest swapping the two:
if (error === WAIT_FOR_URL_CHANGE_TOKEN) {
// prevent moving forward, return a promise that never resolves
// so that the $router can observe the $location update
return Promise.halt();
}
throw error;
@@ -1,6 +1,8 @@ | |||
import _ from 'lodash'; | |||
|
|||
module.exports = class RouteSetupManager { | |||
export const WAIT_FOR_URL_CHANGE_TOKEN = new Error('WAIT_FOR_URL_CHANGE_TOKEN'); |
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.
If I were coming across this code, I'd end up here, where this token is defined, to try to understand why and how to use it. Maybe just leave a comment here, explaining its role?
// Throw this inside of an Angular route resolver after calling `kbnUrl.change` so that the $router can
// observe the $location update. Otherwise, the location won't be updated.
export const WAIT_FOR_URL_CHANGE_TOKEN = new Error('WAIT_FOR_URL_CHANGE_TOKEN');
@spalger This is a really nice elegant solution! I found it really easy to follow and understand. Thanks for CCing me and letting me take a look. |
…f-getting-started-on-navigation
* [tests/functional] automatically opt-out of getting started page on navigation * [test/functional/commonPage] avoid circular reference * [test/functional/commonPage] move check for getting started after url has settled * [gettingStarted] prevent route resolution by returning halt promises * [test/functional/commonPage] wait for kibana to load before checking for getting started * [uiRoutes/setupWork] provide a token that can be thrown to halt setup work * [ui/routes] rename reference to WAIT_FOR_URL_CHANGE_TOKEN * address review feedback (cherry picked from commit c9c8e8d)
5.5/5.x: 3f181d1 |
…11881) * [tests/functional] automatically opt-out of getting started page on navigation * [test/functional/commonPage] avoid circular reference * [test/functional/commonPage] move check for getting started after url has settled * [gettingStarted] prevent route resolution by returning halt promises * [test/functional/commonPage] wait for kibana to load before checking for getting started * [uiRoutes/setupWork] provide a token that can be thrown to halt setup work * [ui/routes] rename reference to WAIT_FOR_URL_CHANGE_TOKEN * address review feedback
When running the functional tests, before finishing navigations, check if the getting started page is visible and if so opt out of it.