Functional testing with elastic-charts

[timroes]

We currently have a couple of functional tests that check on the chart rendered, by crapping it's SVG element, e.g. to get a list of all slices in a pie chart.

We're in the process of switching from our current Vislib implementation to the elastic-charts library for all classical visualizations in Kibana. The library does no longer use SVG, but the HTML canvas element for rendering. With that we can no longer dig deeper into DOM elements inside the chart to get data from it.

In a sync we discussed quickly what potential solutions for this are (and we're assuming that we're not wanting to get rid of any functional test that does that) and came up with the following idea:

elastic-charts should have a debug mode, that when enabled will attach "chart data" (or whatever other information we need) as a data-attribute to the DOM element, so we can retrieve it from there.

What are the benefits? This would still be better than using the inspector data table (as we do in some tests), since there are still quiet a lot of architectural layers between the inspector data panel and the data elastic-chart sees, why we think it's better testing on the data elastic-charts actually see. We can also keep all tests as they are, and just need to replace the page objects to parse the data from this attribute.

What are the drawbacks? Compared to the previous way of testing, we're testing slightly less the actual rendering of the chart. We're fine with that, since this should not be a concern of the functional tests in Kibana. Elastic-charts should be responsible for turning that data into a rendered chart, and thus the tests whether the data is turned into a correct chart should be within elastic-charts (and we have visual regression tests there - and in addition a couple of them in Kibana too).

Open questions

• How will we enable the debug mode in elastic charts? We basically have two options: (a) make it an explicit property on the chart element. This has the advantage of being very explicit, but therefore require us to pass this somehow in all renderers from the test runner into the chart library. Or (b) elastic chart could look at some well known window property (like window.ELASTIC_CHARTS_DEBUG) and enable the mode if that's set to true and the test runner would just set this to true before starting tests. In case of (a) we also need to check how to get that information from the test runner into the chart renderers which would most likely work via a similar well-known window property. I currently have a slight tendency towards (b) but would like to gather your feedback on this.

cc @elastic/kibana-qa @markov00

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[elasticmachine]

Pinging @elastic/datavis (Feature:ElasticCharts)

[markov00]

@timroes I agree with the option b) of the well-known window property, it's easier to implement for Kibana and we don't have to activate it on every chart usage.

[LeeDr]

The change to use elastic-charts was one of the primary reasons for us to invest time in visual testing tools. There are a few advantages to visual testing;

1. Tests the product the same way users will run Kibana (within any debug parameters)
2. Tests every aspect of the chart's appearance (not just points on the chart)

Before we implement another solution, we should check with @spalger on the likelihood we'll get this working;
#52026

[timroes]

@LeeDr While I agree that visual testing tools are def one fundamental pillar of how we want to do testing, I am not sure if we really want to replace all functional tests that currently using the SVG of the chart by a screenshot comparison test? In a lot of places we're really more interested into whether the right slices show up, then if the chart looks correct. If I look at a test suite for a chart type, my personal feeling would be, we should have 2-4 screenshot tests maybe in, but still have a lot more functional tests for checking on a lot of those data edge cases? Would we really want to have 20 screenshots per chart type, and "none" (or close to none) that compares the data shown?

[markov00]

@timroes looking at the current page object for the vislib (bar/area/line) here https://github.com/elastic/kibana/blob/bf04235dae35452061cc7ea3d86d96c19a58206c/test/functional/page_objects/visualize_chart_page.ts most if not all of the function are used to check the correctness of the rendering not the data itself.
Some involve checking the text of the axis labels/ticks (that are perfectly checked via VRTs), some other are checking the bars/line pixel sizes with a weird calculation (they are transforming the data out of the current svg path back to its original form or a "value" that decoupled by the chart height (e.g.

      const data = await path.getAttribute('d');
      const tempArray = data
        .replace('M ', '')
        .replace('M', '')
        .replace(/ L /g, 'L')
        .replace(/ /g, ',')
        .split('L');
      const chartSections = tempArray.length / 2;
      const chartData = [];
      for (let i = 0; i < chartSections; i++) {
        chartData[i] = Math.round((yAxisHeight - Number(tempArray[i].split(',')[1])) * yAxisRatio);
      }

that to me doesn't seem more robust than a VRTs.

For the piechart also we have similar checks where we are checking the number of slices and the rendered labels (both of them easily detected by a VRT).

Using VRTs instead of the current test IMHO is much more robust and we are checking much more than before.
Take this for example:

await PageObjects.dashboard.clickNewDashboard();
      await PageObjects.dashboard.addVisualizations([PIE_CHART_VIS_NAME]);
      await pieChart.expectPieSliceCount(0);

      await PageObjects.timePicker.setHistoricalDataRange();
      await pieChart.expectPieSliceCount(10);

In this case, we are just checking the number of slices, not if the chart is rendered correctly, nor if the labels appear or the pie has enough room to be nicely displayed.

On the other side, I think it will depend on how much time it will take to get back these checks from Percy and if this is fast enough to be replaced seamlessly into our current functional tests or of we have to move all these tests into a different job.

[spalger]

I agree that visual regression testing (VRT) is important to making sure that Kibana functions the way it should, but I would also love to trust that @elastic/charts is well tested enough that we can remove the render testing we're currently doing in Kibana once we change the implementation to use @elastic/charts.

It is pretty complicated to setup VRTs for Kibana, which is why we're still not done with it, and much simpler for a project like @elastic/charts to start using a service like Percy without being coupled to Kibana's progress.

I'm personally a fan of validating that we're passing the right options to charts, and I think that either of @timroes options would be suitable (I'd use an advanced/ui setting for option a, and it would be more resilient to page reloads that way, but it's also a useless setting for users, option b is fine and something you might expect from an off-the-shelf charting library, it's just more fragile and if the framework decides the page needs to be reloaded that state will need to setup again).

Once we have real VRT capability in the Kibana repo (it's happening, but it's not currently blocking anything so it keeps getting bumped by blockers) we can implement some high-level smoke tests for dashboards with many visualizations that could help make sure that Kibana is operating well.

In the meantime I really would love to leverage the focus of the @elastic/charts project to test the rendering before it's installed into Kibana.

[stratoula]

@timroes @markov00 I think we can close it. Functional tests have been created for all the es-charts implementations (Lens, TSVB, xy charts, pie charts) and they work as expected, wdyt?

[markov00]

Ok on closing this for me. We followed a mix of solutions a) and b): we have an explicit debug property that needs to be passed. The passed value is window._echDebugStateFlag and is controlled also by the functional tests