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

Re-enable Firefox functional testing on CI #64159

Merged
merged 8 commits into from
Apr 24, 2020

Conversation

dmlemeshko
Copy link
Member

@dmlemeshko dmlemeshko commented Apr 22, 2020

Summary

Closes #64128

Firefox has an open OMM issue , not allowing us to run the original smoke tagged test suites.

With this PR we cut number of suites till things are stable on CI, in oder to have at least some automated testing in Firefox.

Flakiness check:
oss:firefoxSmoke 25 passed
oss:firefoxSmoke 25 passed
oss:firefoxSmoke 20 passed
xpack:firefoxSmoke 25 passed
xpack:firefoxSmoke 25 passed
xpack:firefoxSmoke 20 passed

In separate PR I will give a chance with starting session for each test suite, check if it helps to run more tests on Firefox and how much time longer execution is taking.

@dmlemeshko
Copy link
Member Author

@elasticmachine merge upstream

@dmlemeshko dmlemeshko marked this pull request as ready for review April 22, 2020 14:27
@dmlemeshko dmlemeshko requested a review from a team as a code owner April 22, 2020 14:27
@dmlemeshko dmlemeshko added release_note:skip Skip the PR/issue when compiling release notes test-firefox v7.8.0 v8.0.0 labels Apr 22, 2020
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

@dmlemeshko could you explain how you decided which of the ML smoke tags to remove (your PR currently removes 10 of the 16 smoke tags). Also, we faced some Firefox flakiness with ML tests a while back and as a result, currently all ML tests (except one small navigation test) have the skipFirefox tag attached on a top-level suite, so removing the smoke tag shouldn't make a difference.
To be clear: I'm fine with removing the smoke tag from the ML tests as I would have to re-evaluate Firefox stability for our tests step by step anyway before I re-enable some of them for Firefox execution. I just like to know some more details.
Removing the smoke from the transform tests is fine for now.

@dmlemeshko
Copy link
Member Author

dmlemeshko commented Apr 23, 2020

@dmlemeshko could you explain how you decided which of the ML smoke tags to remove (your PR currently removes 10 of the 16 smoke tags). Also, we faced some Firefox flakiness with ML tests a while back and as a result, currently all ML tests (except one small navigation test) have the skipFirefox tag attached on a top-level suite, so removing the smoke tag shouldn't make a difference.

@pheyos I should start saying that smoke tag was defined to mark tests that we want to run on Firefox. Now I'm more confident that the naming was wrong because it is confusing: the idea was that we put smoke tag on a small amount of test suites that we are specifically running with Firefox on CI. Still it is an option to run other tests locally. For those tests that not flaky, but just constantly fail on Firefox (e.g. expected values are different for graph vs Chrome run or dragNdrop works differently) we introduced skipFirefox tag.
Ideally there shouldn't be smoke & skipFirefox tags used for the same suites.

The initial agreement was to keep Firefox worker run up to 50 min. So ideally no more suites should be tagged with smoke, unless we add another worker for Firefox.

Because of this OOM issue we are limited up 20-25 min of run before Firefox consumes 4+ GB memory and the process got killed. Finally answering the question: I was cutting smoke tags based on execution time decrease and trying to keep balance between ML and other tests since 20 min is quite small amount.

If you think some tests are more important we can replace them, I'm also thinking about replacing smoke tag with firefox-ci or smth. Thoughts?

To be clear: I'm fine with removing the smoke tag from the ML tests as I would have to re-evaluate Firefox stability for our tests step by step anyway before I re-enable some of them for Firefox execution. I just like to know some more details.

I think even after Mozilla fixes the issue, we won't be able to run all the suites labelled smoke because of 1h limit (maybe 1h 20 min max)

@LeeDr We might need to put details about tags in docs/firefox config to make things clear for everyone, what do you think?

@dmlemeshko dmlemeshko force-pushed the re-enable-firefox-tests branch from 884ebc6 to dbdcb44 Compare April 23, 2020 10:31
@pheyos
Copy link
Member

pheyos commented Apr 23, 2020

Thanks for the details here and in our chat on slack @dmlemeshko!

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Latest ML / Transform tag changes LGTM

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 - I didn't pull and run these locally.

Just a note that the sample data functionality is one area that really causes a huge amount of memory usage and tends to cause failures.

@@ -154,16 +154,16 @@ A test suite is a collection of tests defined by calling `describe()`, and then
Use tags in `describe()` function to group functional tests. Tags include:
* `ciGroup{id}` - Assigns test suite to a specific CI worker
* `skipCloud` and `skipFirefox` - Excludes test suite from running on Cloud or Firefox
* `smoke` - Groups tests that run on Chrome and Firefox
* `includeFirefox` - Groups tests that run on Chrome and Firefox
Copy link

Choose a reason for hiding this comment

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

++ I like the change from smoke to includeFirefox

@dmlemeshko
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@brianseeders brianseeders left a comment

Choose a reason for hiding this comment

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

This seems okay, we'll just watch for failures and potentially disable again if it causes a lot of flakiness.

I can't think of any places where removing the smoke tags will cause a problem... I checked with Liza and she said she doesn't use them.

Pinging @spalger as well since he is the one that disabled the tests

@dmlemeshko dmlemeshko merged commit c0bf910 into elastic:master Apr 24, 2020
dmlemeshko added a commit to dmlemeshko/kibana that referenced this pull request Apr 25, 2020
* bring testing on Firefox back

* disable some tests

* skip more tests

* cut more suites for Firefox

* skip more tests for Firefox

* replace smoke tag with includeFirefox

Co-authored-by: Elastic Machine <[email protected]>
dmlemeshko added a commit that referenced this pull request Apr 25, 2020
* bring testing on Firefox back

* disable some tests

* skip more tests

* cut more suites for Firefox

* skip more tests for Firefox

* replace smoke tag with includeFirefox

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@dmlemeshko dmlemeshko deleted the re-enable-firefox-tests branch January 31, 2022 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes test-firefox v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

re-enable Firefox functional UI tests in CI
6 participants