Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Graph] Fix functional tests #47053

Merged
merged 17 commits into from
Oct 9, 2019

Conversation

flash1293
Copy link
Contributor

Fixes #45322 #45321 #45317

Re-enables Graph functional tests with retries in places where route changes happen (because they are not reliable in all scenarios).

Also accounts for UI changes done in the meantime.

@flash1293 flash1293 added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Oct 1, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@@ -47,67 +46,57 @@ export default function({ getService, getPageObjects }: FtrProviderContext) {
'/blog/wp-admin/',
];

const expectedConnectionWidth: Record<string, Record<string, number>> = {
'/blog/wp-admin/': { wp: 2, blog: 5.51581 },
const expectedConnections: Record<string, Record<string, boolean>> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After syncing with @markharwood it doesn't make sense to validate specific line lengths because the exact numbers can change because of a lot of things.

@@ -122,11 +111,15 @@ export default function({ getService, getPageObjects }: FtrProviderContext) {
const { nodes } = await PageObjects.graph.getGraphObjects();
const circlesText = nodes.map(({ label }) => label);
expect(circlesText.length).to.equal(expectedNodes.length);
expect(circlesText).to.eql(expectedNodes);
circlesText.forEach(circleText => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't validate the order of nodes because it doesn't matter

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 added the Feature:Graph Graph application feature label Oct 2, 2019
@flash1293 flash1293 marked this pull request as ready for review October 2, 2019 07:13
@flash1293 flash1293 requested a review from a team October 2, 2019 07:13
@flash1293
Copy link
Contributor Author

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 requested review from kertal and LeeDr October 2, 2019 09:09
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally, no fails, however, it didn't fail in my environment before

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elastic elastic deleted a comment from elasticmachine Oct 4, 2019
@flash1293
Copy link
Contributor Author

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@flash1293
Copy link
Contributor Author

Jenkins, test this. Flaky unrelated test

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@dmlemeshko
Copy link
Member

dmlemeshko commented Oct 8, 2019

Hey @flash1293
I ran your test locally and it works fine. However, when I run it on CI 20x times in parallel I can see this failure:

 1) graph app
       graph
         should show correct node labels:
     Error: retry.try timeout: Error: retry.try timeout: TimeoutError: Waiting for element to be located By(css selector, [data-test-subj="graph-field-search"])
Wait timed out after 10027ms

jobs:elastic+kibana+pull-request:JOB=x-pack-ciGrou

If it is a timing issue, maybe you replace sleep with smth like

testSubjects.existOrFail('graph-field-search', { timeout: 20000 });

test PR #47555

Upd: looks like await PageObjects.graph.selectIndexPattern('secrepo*'); is not always working

Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - If we find flakiness in this test going forward we need to do 2 things;

  • make sure we have API tests for Graph (maybe with the same data set) and see if we get the exact same response from the API every time
  • trim the test down until it's reliable. It's better to have some functional test of graph than none.

@flash1293
Copy link
Contributor Author

flash1293 commented Oct 9, 2019

@dmlemeshko I tested my stabilization with 20 runs here #47592 , no failures

EDIT: Merged in your test commit on accident, reverting and re-running.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

LGTM. I didn't test it locally, but 20 successful runs on CI is a good sign.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Graph Graph application feature release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0
Projects
None yet
5 participants