Skip to content

Conversation

@LeeDr
Copy link

@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)

// 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
Contributor

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
Contributor

LGTM, only minor comments

@thomasneirynck thomasneirynck removed their assignment Sep 4, 2016
@LeeDr
Copy link
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants