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

Fix flaky security/spaces tests #108088

Merged
merged 4 commits into from
Aug 13, 2021

Conversation

jportner
Copy link
Contributor

@jportner jportner commented Aug 10, 2021

This prevents flaky behavior for integration tests. When an object has
conflicts with multiple destinations, we explicitly sort by updated_at
(desc) and then by id (asc).
Before, we implicitly relied on ES to sort by updated_at (desc), which
seems to work, but if both objects had an identical updated_at then
their sort order could differ depending on some unknown factor.
This unskips the delete space integration tests and changes the
assertions to ignore config objects. Those don't behave like normal
saved objects and we can't always rely on them existing for each test
case.
Also removed some dead code.
@jportner jportner marked this pull request as ready for review August 10, 2021 22:17
@jportner jportner requested review from a team as code owners August 10, 2021 22:17
@jportner jportner enabled auto-merge (squash) August 10, 2021 22:17
@jportner jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 10, 2021
@Bamieh
Copy link
Member

Bamieh commented Aug 11, 2021

@jportner I find running the flaky test runner very useful for making sure my fix removed the flakiness

@jportner
Copy link
Contributor Author

@jportner I find running the flaky test runner very useful for making sure my fix removed the flakiness

Thanks, I've kicked off the runner for my branch (https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/1805/). Both of the problematic tests are in xpack:ciGroup:8 so we only need one runner 💪 I'll reply here / update the PR description when the runner finishes.

@jportner
Copy link
Contributor Author

@Bamieh The flaky test runner passed, this is ready for review.

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines -166 to -171
if (fail409Param === 'ambiguous_conflict_1a1b') {
// "ambiguous source" conflict
error = {
type: 'ambiguous_conflict',
destinations: [getConflictDest(`${CID}1`)],
};
Copy link
Contributor Author

@jportner jportner Aug 13, 2021

Choose a reason for hiding this comment

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

I forgot to mention:

This was dead code from the draft implementation of #75444; I originally had an "ambiguous source conflict" test case, but we decided to randomize object IDs in that scenario instead of throwing an error. At the time, I changed the individual test cases accordingly, but I forgot to rip out this now-unused portion of code from the common suite.

So, I saw it here and decided to remove it now 😄

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing these tests!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@jportner jportner merged commit e35be9d into elastic:master Aug 13, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 13, 2021
@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 13, 2021
Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
@jportner jportner deleted the fix-flaky-security-tests branch January 19, 2022 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment