Handle test failures better

[thomasneirynck]

When a test used to fail, the error used to be swallowed and the done() method would not be called. This caused the entire suite to fail with time-out errors, when a single test failed, because the other tests would not be triggered to run. Resolved this now by explicitly trapping the errors and calling done().

Also improved the error messages for individual assertions.

This may not fix the intermittent errors in CI, but

• it will give us a more meaningful error message when a test failure does happen
• a single failing test will not bring down the entire test suite with timeout errors

---

UPDATE:

Along with tests against 5.1 (#9254) and 5.x (#9255), the tests fail about 1/5 times. They always fail with an identical issue across all branches: one tag is placed at the center, while two other tags are moved away far outside the viewport. The 'wrong' positions are always identical for each test-stub. So the test fails "correctly" on an assertion, and not due to an unexpected error.

b79ec60: this reorganizes the test suite. It makes all tests synchronous, isolating each assertion into its own stub, and moves all bootstrapping of the test into the before-handler. The advantage is that we get better labeling of the tests and their failures, and perhaps we get more consistent behavior. The reasoning for this is fairly thin: if there is latency in the stub, it is now moved to the bootstrapping phase, which may have less of an impact. That said, the reorganization also helps readability because the assertions are no longer nested in callback hell. (@spalger suggested further simplifcations)

7bb150f: removes the time limit for a single cloud-layout on each tick. I don't think this is necessary because we truncate the cloud already to have a manageable number of tags on screen. However, perhaps because of this the layout does not complete in time, causing cloud to be stuck mid-way.

---

UPDATE 2:

f6463b8: Skips the tests when they fail in the particular way outlined above. Failures for any other reason will continue to propagate correctly. This will keep the tests from bringing down the entire test-suite in CI. In the long term, we will need to address a different way, by identifying why this random failure occurs.

The diff of the test-file looks rather scary. Note that the content has not changed, but right now it uses a cleaner syntax (async/await instead of promises + done-callback).

[spalger]

I think these tests would be a lot easier to follow if they were using promises, and as a bonus errors would already be propagating correctly.

[thomasneirynck]

you can return promises?

[spalger]

as an example, I would write this test to look like this:

it(`should position elements correctly: ${index}`, async () => {
  const tagCloud = new TagCloud(domNode);
  tagCloud.setData(test.data);
  tagCloud.setOptions(test.options);

   // fromNode comes from bluebird and turns tagCloud.on('renderComplete')
   // into a promise that resolves once the rendering is complete.
  await fromNode(cb => tagCloud.on('renderComplete', cb));
  
  tagCloud.removeListener('renderComplete', onRender);
  const textElements = domNode.querySelectorAll('text');
  verifyTagProperties(test.expected, textElements);
  expect(tagCloud.getStatus()).to.equal(TagCloud.STATUS.COMPLETE);
});

The same can be done with the timeouts below:

it('...', async () => {
  // ... do some setup ...

  // delay() also comes from bluebird
  await delay(200);

  // ... continue test ...
})

[spalger]

you can return promises?

yep!

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

also got it to fail on master, with exact same errors as 5.0 and 5.x: https://kibana-ci.elastic.co/job/elastic+kibana+pull-request+multijob-intake/2286/console

[thomasneirynck]

identical failtures on master as 5.x: https://kibana-ci.elastic.co/job/elastic+kibana+pull-request+multijob-intake/2292/console

Chrome 54.0.2840 (Linux 0.0.0) tag cloud should position elements correctly: test#: 0 options: {"orientation":"single","scale":"linear","minFontSize":10,"maxFontSize":36} FAILED
	Error: top boundary of tag should have been inside | bbox-of-tag: [1161.5,1300,1170.5,1313] vs
	     bbox-of-container: 512,512
	     debugInfo: {"positions":[["foo",910,1054,0],["bar",910,1054,0],["foobar",0,0,0]],"size":[512,512]}
	    at verifyBbox (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:350802:14)
	    at isInsideContainer (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:350793:6)
	    at http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:350787:8
	    at Array.forEach (native)
	    at verifyTagProperties (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:350776:21)
	    at TagCloud.onRender (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:350533:12)
	    at TagCloud.EventEmitter.emit (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:242830:18)
	    at TagCloud._processQueue (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:241968:15)
	    at resolveWhenDone (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:242056:18)
	    at SVGTextElement.<anonymous> (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:242071:10)
	    at SVGTextElement.event (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:181700:43)
	    at Object.tick [as c] (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:190114:55)
	    at d3_timer_mark (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:183356:74)
	    at d3_timer_step (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=4:183336:16)
Chrome 54.0.2840 (Linux 0.0.0): Executed 662 of 680 (1 FAILED) (0 secs / 3.776 secs)

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

jenkins, test this

[thomasneirynck]

@spalger @ppisljar @jbudz

I would suggest this approach right here, and skip the assertion if we get into this weird layout state. I'm not sure what is causing the flipping-tests. It is annoying, and is harming our CI. But we'll also need more time to investigate what's really causing the error, and this issue may (or may not) be upstream in d3-tagcloud. I will create a separate issue for this. If we can merge this, at least other people's PR will not be affected.

[ppisljar]

I agree with Thomas, this is a good short term solution we need as this is affecting our whole CI. Investigating what exactly causes this and fixing it might require more time than it is worth.

[thomasneirynck]

jenkins, test this

[jbudz]

👍 on getting things green, I think this is okay until we get to the root cause.