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

Rewrite Grok Debugger Tests to use Standard Functional Test Runner #108301

Merged
merged 9 commits into from
Aug 17, 2021

Conversation

cuff-links
Copy link
Contributor

@cuff-links cuff-links commented Aug 11, 2021

This functional test addresses the constant failures that happen with the grok debugger tests. These tests have been rewritten to use standard functional test runner practices, which they weren't before. This is in hopes to address the consistent flakiness of these tests.

@cuff-links
Copy link
Contributor Author

Fixed the Grok debugger tests and ran through flaky test runner. https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/1810/

@cuff-links cuff-links marked this pull request as ready for review August 14, 2021 07:50
@cuff-links cuff-links added Feature:Grok Debugger Dev Tools Grok Debugger feature Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Aug 14, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

@cuff-links cuff-links added v7.15.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.14.1 v8.0.0 and removed v7.15.0 labels Aug 14, 2021
@cuff-links
Copy link
Contributor Author

Also an unrelated failure in a different test group.

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Nice job @cuff-links! Great to see these tests refactored and converted to TS. Ran tests successfully locally.

I left a few comments/questions in the code that I'd like to get more information on before approving. Also, is there still an open issue for these flaky tests? If so, can you link to it in the PR description?

expect(response).to.eql(testData);
});

// This test will need to be fixed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what's wrong with this test and why we need to still skip it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The UI has changed since this test was last working. I will need to actually rewrite this test from the ground up. The current PR is just about migrating the existing tests to use current conventions to aid with flakiness. So fixing this other test should be fixed as part of a separate since that wasn't the purpose of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! As discussed 1:1, if you can open a separate issue to address this so we do lose track of it that'd be great. It'd also be helpful if you can reference the issue link in the code comment.

@alisonelizabeth
Copy link
Contributor

@cuff-links I believe this PR should also be tagged with v7.15.0

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM. Thanks @cuff-links! Added a note about opening an issue to address the skipped test.

@cuff-links
Copy link
Contributor Author

New issue opened #108814

@cuff-links cuff-links merged commit 9aa1705 into elastic:master Aug 17, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 17, 2021
…lastic#108301)

* Converted Grok debugger tests to use standard functional test runner practices.

* Fixed last test.

* Removed old test now that the new test is passing.

* Fixed condition for retry.

* Removed .only to restore test run.

* fixed errors

* Fixed nits.
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 17, 2021
…108301) (#108815)

* Converted Grok debugger tests to use standard functional test runner practices.

* Fixed last test.

* Removed old test now that the new test is passing.

* Fixed condition for retry.

* Removed .only to restore test run.

* fixed errors

* Fixed nits.

Co-authored-by: John Dorlus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Grok Debugger Dev Tools Grok Debugger feature release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants