Skip to content

Add test for order aggs by term #8141#8155

Merged
LeeDr merged 2 commits intoelastic:4.6from
LeeDr:testOrderAggsByTerm8141
Sep 5, 2016
Merged

Add test for order aggs by term #8141#8155
LeeDr merged 2 commits intoelastic:4.6from
LeeDr:testOrderAggsByTerm8141

Conversation

@LeeDr
Copy link
Copy Markdown

@LeeDr LeeDr commented Sep 2, 2016

Manual backport of #8154 to 4.6.
Added new test to _line_chart.js that changes Order By to Term. It passes on 4.6 branch master with the fix (4.6.1), but fails on 4.6.0 with these messages (I trimmed out some stack trace);

>> FAIL: chrome on any platform - visualize app - visualize app - line charts - should show correct chart order by Term (2772ms)
Error: expected false to be truthy
10:53:29.029: Taking screenshot "C:\git\4.x\test\screenshots\failure\failure_1472831609029_line charts_should show correct data, ordered by Term.png"
>> FAIL: chrome on any platform - visualize app - visualize app - line charts - should show correct data, ordered by Term (935ms)
Error: Error: expected [ 'jpg 9,109',
  'css 2,159',
  'png 1,373',
  'gif 918',
  'php 445' ] to sort of equal [ 'png 1,373',
  'php 445',
  'jpg 9,109',
  'gif 918',
  'css 2,159' ]
10:53:29.399: Start of testLineChartVisualization
10:53:30.455: saveButton button clicked

This line would normally be the successful save notification but shows the error instead;

10:53:30.708: Saved viz message = Visualize: Cannot read property 'type' of undefined

10:53:30.710: Taking screenshot "C:\git\4.x\test\screenshots\failure\failure_1472831610710_line charts_should be able to save and load.png"
>> FAIL: chrome on any platform - visualize app - visualize app - line charts - should be able to save and load (1849ms)

@cjcenizal cjcenizal self-assigned this Sep 2, 2016
@thomasneirynck thomasneirynck self-assigned this Sep 2, 2016
thomasneirynck
thomasneirynck reviewed Sep 4, 2016
View reviewed changes
test/functional/apps/visualize/_line_chart.js
// https://github.com/elastic/kibana/issues/8141
var expectedChartData = ['png 1,373', 'php 445', 'jpg 9,109', 'gif 918', 'css 2,159'];

common.debug('Order By = Term');
Copy link
Copy Markdown
Contributor

@thomasneirynck thomasneirynck Sep 4, 2016

Choose a reason for hiding this comment

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

this is more personal opinion, so whatever you do with this is totally at your discretion.

IMHO, I find the logging statements not necessary. The tests are quite constrained in scope, and a failure should very quickly point to the spot where things go wrong. To me, they help at dev-time, but can be removed once checked in.

@thomasneirynck
Copy link
Copy Markdown
Contributor

thomasneirynck commented Sep 4, 2016

LGTM, only minor comments

@thomasneirynck thomasneirynck removed their assignment Sep 4, 2016
@LeeDr
Copy link
Copy Markdown
Author

LeeDr commented Sep 5, 2016

I eliminated a lot of unused 'remote' vars through the visualize tests.

@LeeDr LeeDr merged commit d578e27 into elastic:4.6 Sep 5, 2016
@cjcenizal cjcenizal removed their assignment Sep 6, 2016
@LeeDr LeeDr deleted the testOrderAggsByTerm8141 branch May 23, 2017 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

review test v4.6.1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LeeDr @thomasneirynck @cjcenizal