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.

Sign up for GitHub

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

Jump to bottom

update spy table headers when columns update #13130

Merged
merged 4 commits into from
Jul 31, 2017
Merged

update spy table headers when columns update #13130

merged 4 commits into from
Jul 31, 2017

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jul 26, 2017 •
edited
Loading

fixes #7211

backported to 6.x: #13224
backported to 6.0: #13225

Release Note:
Ensure spy panel table headers update when new aggregation data is fetched

@nreese nreese added :Sharing Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix labels Jul 26, 2017
stacey-gammon
stacey-gammon reviewed Jul 26, 2017
View reviewed changes
test/functional/page_objects/visualize_page.js Outdated
.setFindTimeout(defaultFindTimeout * 2)
.findByCssSelector('table.table.table-condensed thead')
.getVisibleText();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make this:

async getDataTableHeaders() {
  const dataTableHeader = await retry.try(async () => await find.byCssSelector('table.table.table-condensed thead'));
  return await dataTableHeader.getVisibleText();
}

I'm in the process of converting most of the functions in this file over to that style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I can. Looks like find is not defined

tryForTime failure: find is not defined

Copy link
Contributor

Choose a reason for hiding this comment

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

Add const find = getService('find'); at the top of the page

@stacey-gammon
Copy link
Contributor

hmmm, might be a flaky test:

08:24:50.596          │ debg  TestSubjects.find(globalLoadingIndicator)
08:24:50.597          │ debg  in displayedByCssSelector: [data-test-subj~="globalLoadingIndicator"]
08:24:53.022          │ debg  Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/screenshots/failure/visualize app tile map visualize app tile map chart Fit data bounds should zoom to level 3.png"
08:24:53.195        └- ✖ fail: "visualize app tile map visualize app tile map chart Fit data bounds should zoom to level 3"

Not sure why the values differ, rather than a stale element error, in this failure though. lets see if it passes the second time.

jenkins, test this

@nreese
Copy link
Contributor Author

nreese commented Jul 26, 2017

Passed a second time. Lets see what happens on another round
jenkins, test this

@stacey-gammon
Copy link
Contributor

Must be a flaky test, I just had the same failure in #13034. Will see what I can do.

stacey-gammon
stacey-gammon approved these changes Jul 27, 2017
View reviewed changes
Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

lgtm!

thomasneirynck
thomasneirynck reviewed Jul 27, 2017
View reviewed changes
src/ui/public/paginated_table/paginated_table.html Outdated
@@ -8,7 +8,7 @@
<thead>
<tr>
<th
ng-repeat="col in ::columns"
ng-repeat="col in columns"
Copy link
Contributor

Choose a reason for hiding this comment

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

dayum....

thomasneirynck
thomasneirynck approved these changes Jul 27, 2017
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

yup!

@thomasneirynck
Copy link
Contributor

merge conflict was pretty easy, some methods got interleaved.

spalger
spalger suggested changes Jul 28, 2017
View reviewed changes
test/functional/page_objects/visualize_page.js Outdated
@@ -776,6 +777,11 @@ export function VisualizePageProvider({ getService, getPageObjects }) {
.getAttribute('height');
}

async getDataTableHeaders() {
const dataTableHeader = await retry.try(async () => await find.byCssSelector('table.table.table-condensed thead'));
return await dataTableHeader.getVisibleText();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please not add more CSS selectors to the functional tests and instead add data-test-subj attributes to selection targets and use the testSubjects service?

@nreese nreese force-pushed the spy_table_headers branch from 01df37f to ddc7519 Compare July 31, 2017 14:44
@nreese
Copy link
Contributor Author

nreese commented Jul 31, 2017

jenkins, test this

@nreese nreese merged commit abcc055 into elastic:master Jul 31, 2017
nreese added a commit to nreese/kibana that referenced this pull request Jul 31, 2017
@nreese
update spy table headers when columns update (elastic#13130)
4ad2873 
* update paginated table headers with columns update

* remove unneeded sleep

* update getDataTableHeaders to preffered format

* use data-test-subj attribute for functional tests instead of CSS selectors
@nreese nreese added v7.0.0 and removed v6.0.0 labels Jul 31, 2017
@nreese nreese mentioned this pull request Jul 31, 2017
Merged
nreese added a commit to nreese/kibana that referenced this pull request Jul 31, 2017
@nreese
update spy table headers when columns update (elastic#13130)
5b8f52e 
* update paginated table headers with columns update

* remove unneeded sleep

* update getDataTableHeaders to preffered format

* use data-test-subj attribute for functional tests instead of CSS selectors
@nreese nreese mentioned this pull request Jul 31, 2017
Merged
nreese added a commit that referenced this pull request Aug 2, 2017
@nreese
update spy table headers when columns update (#13130) (#13224)
2fb7973 
* update paginated table headers with columns update

* remove unneeded sleep

* update getDataTableHeaders to preffered format

* use data-test-subj attribute for functional tests instead of CSS selectors
nreese added a commit that referenced this pull request Aug 2, 2017
@nreese
update spy table headers when columns update (#13130) (#13225)
9a34c2f 
* update paginated table headers with columns update

* remove unneeded sleep

* update getDataTableHeaders to preffered format

* use data-test-subj attribute for functional tests instead of CSS selectors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stacey-gammon stacey-gammon stacey-gammon approved these changes

@thomasneirynck thomasneirynck thomasneirynck approved these changes

@spalger spalger spalger approved these changes

Assignees
No one assigned
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change to Visualization Y-Axis aggregation not reflected in data table
4 participants
@nreese @stacey-gammon @thomasneirynck @spalger