[Graph] Re-enable Functional Tests

[elasticmachine]

Original comment by @tsullivan:

From Slack:

verynomagic [8:32 AM]
@courtewing just curious, were you seeing the same kind of failure? graph app graph should show correct data circle text showing 9 instead of 15?
courtewing [8:32 AM]
yep
[8:32]
it's because the graph is not showing a bunch of ip addresses that are there in the data
[8:32]
under the src field

From the console log in CI:

16:39:55 proc [ftr] ?- ? fail: "graph app graph should show correct data circle text"
16:39:55 proc [ftr] ?- ? fail: "graph app graph should show correct number of connecting lines"
16:40:08 proc [ftr] ?- ? fail: "graph app graph should open Graph workspace"
16:42:08 proc [ftr] ?- ? fail: "graph app graph should show venn when clicking a line"

[elasticmachine]

Original comment by @tsullivan:

cc @epixa @markharwood @archanid

[elasticmachine]

Original comment by @markharwood:

TLDR - I couldn't reproduce this.
@epixa shared a LINK REDACTED with the old commit points that were "OK".
I ran the queries on this version and master (as of Monday 27th Nov) and the results for the elasticsearch queries were identical. Admittedly the master querying run was done on the index built in the previous run based on the old commit points so maybe there's an issue with indexing using master.
The queries used in the gist were my interpretation of what the Graph API produces as underlying elasticsearch queries. To ensure I hadn't got this wrong I ran the example query through the GUI on master:

It has the required 15 nodes not the 9 reported in this error.
Transcript of steps taken:

• synced to the “last working” commit points in the gist.
• Started es with a clean data dir and on port 9240 (looked to be what the selenium test scripts wanted to access)
• started Kibana on 5601 pointing at es 9240
• Ran selenium test script to create the secrepo index
• Ran our 2 tests queries from console, capturing the JSON response

Then I shutdown all the above processes and synced to master on all repos. To run es I noticed this required a new Lucene download.

• Started es using previously-built data dir but now on port 9200
• started Kibana on 5601 pointing at es 9200
• Re-ran last 2 queries capturing the JSON response.

The 2 responses were identical (other than took time in millis).

The Graph UI screenshot was taken from master doing the following:

• New workspace looking at secrepo
• Select fields url, url.parts, src, params with default settings etc
• ran a query for “admin”

[elasticmachine]

Original comment by @epixa:

@markharwood Hmm... I'll attempt to reproduce again then. I just checked the latest master of everything via CI and it's failing in the same way as described in this issue: LINK REDACTED

[elasticmachine]

Original comment by @LeeDr:

I haven't run the tests lately to see if anything's changed. But I'm thinking even if the results have changed we should update the tests to match the current results and keep the tests running. Otherwise we won't even know if the results change again (or change back).

[elasticmachine]

Original comment by @epixa:

I haven't had a chance to reproduce this one again. @LeeDr my concern here is that there's still some ambiguity around what is happening here. I was able to reproduce the same problems encountered on CI locally, but @markharwood was not. So basically, we can update the test so it passes on CI, which effectively means we update the test to reflect a breaking change in ES graph, which is fine because this is a major version. However, what I'd be updating the test to is a state that Mark can't even reproduce, which is an awkward place for our tests to be especially since he's the primary contributor to graph.

[elasticmachine]

Original comment by @epixa:

To clarify: disabling these tests only went into master because the issue here seems to be a breaking change in graph in master or at least some inconsistency with the returned data in master under certain conditions. I am not aware of this issue ever having occurred in 6.x.

Edit: the ci jobs are long since gone, so just noting here that the test failure was along the lines of "expected 15 nodes but got 9". Any other graph issues we encounter in CI on other branches and whatnot is not the same failure that prompted this.

[elasticmachine]

Original comment by @markharwood:

I reproduced this now on master.
Is it possible the secrepo index used in the tests changed from being a 1 shard index to a 5 shard one?
I notice that a possible reason for there being less vertices produced in results is that some low-frequency vertices of potential interest are overlooked when the data is distributed across multiple Lucene indices. I get exactly 15 vertices back when I recreate the conditions of a single-shard index (setting shard_min_doc_count:1 ensures all shards return all terms - the default is 2).

Generally speaking, you can expect there to be the possibility of deviations in several aggregations if you change the number of shards used in a test index.

[elasticmachine]

Original comment by @epixa:

@markharwood It looks like it has been 5 shards since this test was created in LINK REDACTED

If this is a shard-related issue, then it seems like the Kibana tests are too focused on quality testing for an imprecise scenario. If 1 shard is reliably precise, then I think we should update the test fixture to have number_of_shards of 1. Thoughts?

[elasticmachine]

Original comment by @markharwood:

I can confirm that changing LINK REDACTED to use only 1 shard allows this test to move on past the 9!=15 issue with counting vertices.
However, it still fail with tests graph app graph should open Graph workspace and graph app graph should show venn when clicking a line

[elasticmachine]

Original comment by @epixa:

I'm not sure what the venn issue is, but the open issue looks to be a different problem that exists on 6.x as well: LINK REDACTED

[elasticmachine]

Original comment by @epixa:

I pushed a fix for the shard count here: LINK REDACTED

[elasticmachine]

Original comment by @markharwood:

I'm not sure what the venn issue is

A concerning aspect of the test is that it is trying to identify DOM elements with some very specific properties that are derived by scoring logic that could change subtly e.g.

await PageObjects.graph.clickGraphConnectingLine('stroke-width:2.0042343137225673px');

That's likely to be pretty brittle. Finding the line that connects circle labelled termA with circle labelled termB would be safer (if possible to declare)

[elasticmachine]

Original comment by @epixa:

Arg, that change is causing this test fail on 6.x/6.2 branches, so I'm going to revert it there until I can figure out why.

[elasticmachine]

Original comment by @markharwood:

What was the 6.x failure?

[elasticmachine]

Original comment by @tylersmalley:

CI failure here: LINK REDACTED

[elasticmachine]

Original comment by @LeeDr:

In that ^ CI failure, it is finding the line with that exact stroke-width:2.0042343137225673px but failing to click on it because </line> is not clickable at point (744, 444). Other element would receive the click:. Which means that there's a circle on top of the center point of the line where it's trying to click.

Although all the parameters of the graph are the same each time, the actual layout of the graph isn't deterministic. So sometimes that line is covered and sometimes it's not. So I coded the test to retry drawing the whole thing over and over until it (hopefully) can click that line and get to the Venn diagram.

I can see in that Jenkins job that it failed 11 times before the test timed out;
is not clickable at point (543, 530)
is not clickable at point (501, 441)
is not clickable at point (785, 384)
is not clickable at point (778, 370)
etc.

We could remove some tests, but by clicking that line and getting to the Venn diagram this test was checking a case that had previously shipped as a regression.

[elasticmachine]

Original comment by @tylersmalley:

@markharwood @elastic/kibana-visualizations this is a blocker for 6.3. Can someone look into these test failures and re-enable the Graph tests?

[elasticmachine]

Original comment by @timroes:

I can have a look into that, but I am not very positive, that - looking at the past conversation here and that Mark already looked into that - I am able to fix this before FF properly. Also why is this a blocker? The tests are disabled at the moment as far as I understand, and this issue is about reopening and fixing failing tests, that have been failing for at least half a year according to this issue?

[elasticmachine]

Original comment by @timroes:

@epixa @tylersmalley I've tried to reproduce this failures, and I can't get any of these test to run stable even locally. As @LeeDr pointed out it seems, the graphs doesn't seem to be generated 100% deterministic. In that case I don't think that any kind of test on this, that actually uses this super precise pixel width filtering would make any sense. Trying to redraw a graph multiple times until it has the "right" visual, that will work with the test is imho nothing that we can build any test on.

So I really would like to discuss if this should be a blocker at the moment. If so, I think I need Lee and Marks help in basically rewriting the tests.

[elasticmachine]

Original comment by @epixa:

@timroes This is a visualization team thing, so if you don't think this is a blocker, then it probably isn't. Not having any tests on this functionality isn't great though, so perhaps there's some portion of the tests we can remove if they aren't appropriate while leaving the rest in place?

[elasticmachine]

Original comment by @timroes:

In that case I will remove the blocker status of this. I totally agree with you that it's not a good thing not having tests on these. I would at the moment rather not enable any of those, even though I suspect that one of it is rather stable. But due to the very close feature freeze and the importance of that feature freeze, I don't want to (re)introduce tests now, we know are potentially flaky.

If we consider these tests cases to be very important I would rather fix them after the repository merge next week instead, as long as @LeeDr and @markharwood have the time to dig into this together.

@LeeDr please feel free to Veto against that, if you can't bear that decision from QA team perspective.

[elasticmachine]

Original comment by @markharwood:

To address my concerns over LINK REDACTED I imagine a more robust solution is to provide stable id attributes for <line>s that contain a combination of source and target node labels. These would have no practical use in normal operation so I wonder if there's a "test mode" flag available somewhere that could be used to turn this labelling behaviour on.

[elasticmachine]

Original comment by @LeeDr:

When we re-enable these tests, also fix this issue;
LINK REDACTED

[elasticmachine]

Original comment by @LeeDr:

I tried to update the test to work on master here LINK REDACTED
but the results are inconsistent.
I just did an experiment where I changed the test to not reload the secrepo test data each time, and then the results WERE consistent. But if I wipe the data and reload it each time I get various different results in the line weights and other areas that makes it extremely difficult to automate a test.

I'm thinking I need to take a step back from the UI and run some graph api tests to see if they return consistent results even when the data is reloaded for each run. Maybe @markharwood could give me some pointers on getting that going.

[rashmivkulkarni]

removing 6.3.2 label from this. FF for 6.3.2 is July 17th and it won't make it.

[stacey-gammon]

cc @elastic/kibana-app