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

Expand coverage of dashboard tests #17703

Merged

Conversation

stacey-gammon
Copy link
Contributor

I created new test data from the 6.x branch so it's correctly labeled as 6.3. I created new visualizations for every type we have so we don't have to spend time creating these visualizations during the tests. I'm also doing some ad hoc rendering/snapshot testing by checking on certain css classes. A bit fragile but not as fragile as the actual visual regression testing, and much faster.

@stacey-gammon stacey-gammon added test Feature:Dashboard Dashboard related features :Sharing labels Apr 13, 2018
@stacey-gammon stacey-gammon force-pushed the 2018-04-update-dashboard-test-data branch 2 times, most recently from 4d9c1c1 to 4703433 Compare April 13, 2018 22:08
@stacey-gammon stacey-gammon changed the title Expand coverage of dashboard tests and decrease time Expand coverage of dashboard tests Apr 13, 2018
@stacey-gammon stacey-gammon force-pushed the 2018-04-update-dashboard-test-data branch 3 times, most recently from c03c37f to cc4122c Compare April 16, 2018 14:54
@stacey-gammon stacey-gammon force-pushed the 2018-04-update-dashboard-test-data branch 12 times, most recently from bc46a6a to 6081dbd Compare April 18, 2018 22:40
@stacey-gammon stacey-gammon force-pushed the 2018-04-update-dashboard-test-data branch 6 times, most recently from 99a7145 to a8c6f25 Compare April 25, 2018 12:20
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@chrisdavies chrisdavies left a comment

Choose a reason for hiding this comment

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

Dang. That's a lot of work. This looks great to me. I really like the helper classes you put together to consolodate common test functionality. I may steal a page from this book in my migrations testing!

@@ -10,6 +10,7 @@ exports[`DashboardPanel matches snapshot 1`] = `
>
<div
class="panel-heading"
data-test-subj="dashboardPanelHeading-"
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it's missing info after the - was it supposed to have some value there, like the title? Probably not important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't a title in the test data for this, but I can add it in so it tests that the title is there and that it doesn't look like a mistake.

async setCustomPanelTitle(customTitle, panel) {
log.debug(`setCustomPanelTitle(${customTitle}, ${panel})`);
await this.openPanelOptions(panel);
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, this is the only place in this entire file that jsdoc comments are used. I'd personally get rid of them, and just add a descriptive // comment if you think one is necessary. Buuuut that's maybe terrible advice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we move towards typescript, these kinds of comments will become more prolific. This does look inconsistent, but if the function deserves a comment (I thought it did, originalTitle doesn't obviously reflect that it's being used to find the panel to change, and that it's optional), I think it might as well be jsdoc vs //. Appreciate the feedback, but I think I'll leave this one here.

/**
*
* @param field
* @param operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. The comment block is fairly useless except for the inputType clarification, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree on this one, I'll remove. not to mention the variable name is wrong :).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon stacey-gammon force-pushed the 2018-04-update-dashboard-test-data branch from c4c7d36 to fd068b8 Compare April 30, 2018 14:53
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

This looks great. I really like how it breaks the functional tests into smaller files.

@@ -0,0 +1,14 @@
export function TimePickerProvider({ getService }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is never used anywhere

async getFilterFieldIndexPatterns() {
const indexPatterns = [];
const groups = await find.allByCssSelector('.ui-select-choices-group-label');
console.log('found ' + groups.length + ' index pattern group labels');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use log server?

});

after(async function () {
console.log('showing chrome again');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use log service?

@stacey-gammon stacey-gammon force-pushed the 2018-04-update-dashboard-test-data branch from fd068b8 to 26a7bdf Compare April 30, 2018 17:00
@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor Author

Failed on:

18:18:58        │ info  Current URL is: http://localhost:5620/app/kibana#/management/kibana/objects?_g=()&_a=(tab:searches)
18:18:58        │ info  Saving page source to: /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/kibana/test/functional/failure_debug/html/management import objects should handle saved searches and objects with saved searches properly.html
18:18:58      └- ✖ fail: "management import objects should handle saved searches and objects with saved searches properly"
18:18:58      │        Error: expected 0 to equal 1
18:18:58      │         at Assertion.assert (node_modules/expect.js/index.js:96:13)
18:18:58      │         at Assertion.be.Assertion.equal (node_modules/expect.js/index.js:216:10)
18:18:58      │         at Assertion.(anonymous function) [as be] (node_modules/expect.js/index.js:69:24)
18:18:58      │         at Context.<anonymous> (test/functional/apps/management/_import_objects.js:128:48)
18:18:58      │         at <anonymous>:null:null

Looks to be a flaky test:
screen shot 2018-05-01 at 7 54 34 am

Filed #18682

Will rebase and test again.

@stacey-gammon stacey-gammon force-pushed the 2018-04-update-dashboard-test-data branch from 26a7bdf to 961e82c Compare May 1, 2018 11:57
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon stacey-gammon merged commit 3c8c23c into elastic:master May 1, 2018
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request May 1, 2018
* Expand coverage of dashboard tests and decrease time

* Fix timing error when sub urls fail to save from too fast app link clicking

* discover doesn't have breadcrumbs

* Check top nav text so it works on both listing and saved object edit/view pages

* need to do the add panel operations one at a time

* Need both types of input in filter

* Give test data a title

* Remove incorrect and unnecessary comment

* Move data around and get rid of 6_3 specific naming as we will end up migrating the data as we progress

* Remove code accidentally checked in
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request May 7, 2018
* Expand coverage of dashboard tests and decrease time

* Fix timing error when sub urls fail to save from too fast app link clicking

* discover doesn't have breadcrumbs

* Check top nav text so it works on both listing and saved object edit/view pages

* need to do the add panel operations one at a time

* Need both types of input in filter

* Give test data a title

* Remove incorrect and unnecessary comment

* Move data around and get rid of 6_3 specific naming as we will end up migrating the data as we progress

* Remove code accidentally checked in
stacey-gammon added a commit that referenced this pull request May 7, 2018
* Expand coverage of dashboard tests and decrease time

* Fix timing error when sub urls fail to save from too fast app link clicking

* discover doesn't have breadcrumbs

* Check top nav text so it works on both listing and saved object edit/view pages

* need to do the add panel operations one at a time

* Need both types of input in filter

* Give test data a title

* Remove incorrect and unnecessary comment

* Move data around and get rid of 6_3 specific naming as we will end up migrating the data as we progress

* Remove code accidentally checked in
@stacey-gammon stacey-gammon deleted the 2018-04-update-dashboard-test-data branch June 11, 2019 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants