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

[8.x] Security solutions upgrade test fixes #130750

Merged
merged 11 commits into from
Apr 27, 2022

Conversation

liza-mae
Copy link
Contributor

Fixes https://github.com/elastic/elastic-stack-testing/issues/1224. Latest report shows these are now passing: https://internal-ci.elastic.co/view/Stack%20Tests/job/elastic+estf-cloud-upgrade-tests/225/testReport/

I commented out a couple of checks and added a TODO since they either in need of a data-test-subj or the verification was not working for some reason and it needs to be investigated, a summary is below.

We can keep these steps commented out, attempt to fix or remove them.

For import_case.spec.ts

  • Some verification steps need data-test-subj added, as the ones referenced do not exist
  • Unclear about the verification of the overview host name, I did not see this field on the page

For threshold_rule.spec.ts

  • Some verification steps need data-test-subj added, as the ones referenced do not exist
  • Verification of participants is not consistent run by run, I am not sure why

@liza-mae liza-mae added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.2.0 v8.3.0 labels Apr 20, 2022
@liza-mae liza-mae requested a review from MadameSheema April 20, 2022 21:17
@liza-mae liza-mae self-assigned this Apr 20, 2022
@liza-mae liza-mae requested review from a team as code owners April 20, 2022 21:17
@liza-mae
Copy link
Contributor Author

@elasticmachine merge upstream

@liza-mae
Copy link
Contributor Author

Looks like this is possibly unrelated, this PR may fix it: #130771

@liza-mae
Copy link
Contributor Author

@elasticmachine merge upstream

@liza-mae
Copy link
Contributor Author

Looks like all the tests pass now, @MadameSheema can you give this a review please?

cy.get(HOST_NAME).should('have.text', alert.hostName);
cy.get(REASON).contains(expectedReason);
// TODO: Needs data-test-subj
// cy.get(HOST_NAME).should('have.text', alert.hostName);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please confirm In which version are you missing a data-test-subj? I checked 8.x and the data-test-subj value is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MadameSheema I added a comment TODO: Needs data-test-subj to the ones that were missing. There are ones in import_cases.spec.ts and threshold_rule.spec.ts and it will be the element below the comment that needs it.

@MadameSheema
Copy link
Member

Thanks for trying to fix the tests @liza-mae. I checked and all the data-test-subj exists on the code but may be updated on the tests. Please let me know if you are ok with me pushing changes on your PR to fix the tests. Thanks.

@liza-mae
Copy link
Contributor Author

@elasticmachine merge upstream

@liza-mae
Copy link
Contributor Author

@MadameSheema yes feel free to push any changes.

Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

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

Thanks for all the efforts done here, really appreciate it ❤️

@liza-mae
Copy link
Contributor Author

Rerunning the cloud upgrade tests with latest changes, pending results.

@liza-mae
Copy link
Contributor Author

@MadameSheema unfortunately the results failed with re-enabling some of the assertions: https://internal-ci.elastic.co/view/Stack%20Tests/job/elastic+estf-cloud-upgrade-tests/228/

@liza-mae
Copy link
Contributor Author

@MadameSheema I reverted the last commit. Let me know how you want to proceed, shall we try to fix these failing assertions in this PR or do another PR later?

@MadameSheema
Copy link
Member

@liza-mae merge it to unblock the current issue and we can continue working on a separate PR in order to bring the assertions back. Can you please let me know which is the upgrade path that you used (original version > new version) for executing the tests? I executed the tests on my local before pushing the latest changes and everything seemed to work, so I guess the original version is going to be important to bring all the assertions back :)

@liza-mae
Copy link
Contributor Author

@MadameSheema sounds good, the report link shows the failing jobs testing this PR: https://internal-ci.elastic.co/view/Stack%20Tests/job/elastic+estf-cloud-upgrade-tests/228/ which can be mapped to versions here: https://raw.githubusercontent.com/elastic/elastic-stack-testing/main/ci/upgrade/ess/upgrade_paths.json.

In this description I wrote what I saw as inconsistent behavior with asserting participants, so I can refile another issue with the details to get the commented assertions added back. Thanks!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @liza-mae

@liza-mae
Copy link
Contributor Author

Just for reference, here are the results from the revert: https://internal-ci.elastic.co/view/Stack%20Tests/job/elastic+estf-cloud-upgrade-tests/lastCompletedBuild/testReport/ Good results - There are only a handful of failures now, which I believe are due to flaky tests, so we can address those along with adding back the removed assertions in separate issues/PRs.

@liza-mae liza-mae merged commit 2c9f2aa into elastic:main Apr 27, 2022
@liza-mae liza-mae deleted the liza/ss-upgrade-fixes branch April 27, 2022 20:55
kibanamachine pushed a commit that referenced this pull request Apr 27, 2022
* Fix upgrade tests

* More fixes

* Remove unused

* Comment out ones missing data-test-subj

* Remove participant verification

* Remove unused vars

* brings back assertions

* Revert "brings back assertions"

This reverts commit 5783eae.

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Gloria Hornero <[email protected]>
(cherry picked from commit 2c9f2aa)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.2

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Apr 27, 2022
* Fix upgrade tests

* More fixes

* Remove unused

* Comment out ones missing data-test-subj

* Remove participant verification

* Remove unused vars

* brings back assertions

* Revert "brings back assertions"

This reverts commit 5783eae.

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Gloria Hornero <[email protected]>
(cherry picked from commit 2c9f2aa)

Co-authored-by: liza-mae <[email protected]>
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
* Fix upgrade tests

* More fixes

* Remove unused

* Comment out ones missing data-test-subj

* Remove participant verification

* Remove unused vars

* brings back assertions

* Revert "brings back assertions"

This reverts commit 5783eae.

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Gloria Hornero <[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 release_note:skip Skip the PR/issue when compiling release notes v8.2.0 v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants