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.

Sign up for GitHub

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

Jump to bottom

Fix random e2e failure. #534

Merged
merged 1 commit into from
Jul 9, 2020
Merged

Fix random e2e failure. #534

merged 1 commit into from
Jul 9, 2020

Conversation

pratikjagrut
Copy link
Contributor

@pratikjagrut pratikjagrut commented Jun 30, 2020 •
edited

Motivation

fix E2E failure

Changes

  • Seed random with time.Now to generate unique digits on every subtest call.

  • Replace retry code block with require.Eventually.

  • Remove test timeout from makefile test-e2e target.

  • Update github.com/stretchr/testify from v1.4.0 to v1.6.1
    The 1.4.0 version we were using had a bug in the Eventually function,
    which caused flaky results.

  • Increasing timeout for Eventually function

  • Update vendor dependencies.

  • Updating test timeout.

Testing

make test-e2e

@pratikjagrut
Copy link
Contributor Author

/retest

1 similar comment
@pratikjagrut
Copy link
Contributor Author

/retest

@pratikjagrut
Copy link
Contributor Author

/test e2e
/test 4.5-e2e

3 similar comments
@pratikjagrut
Copy link
Contributor Author

/test e2e
/test 4.5-e2e

@pratikjagrut
Copy link
Contributor Author

/test e2e
/test 4.5-e2e

@pratikjagrut
Copy link
Contributor Author

/test e2e
/test 4.5-e2e

@pratikjagrut
Copy link
Contributor Author

/retest

@pmacik pmacik mentioned this pull request Jul 3, 2020
Closed
@pratikjagrut
Copy link
Contributor Author

/test e2e

@pratikjagrut
Copy link
Contributor Author

/test e2e
/test 4.5-e2e

@pratikjagrut
Copy link
Contributor Author

/test 4.5-e2e

@pratikjagrut
Copy link
Contributor Author

/test lint
/test unit

@pratikjagrut
Copy link
Contributor Author

/retest

@pratikjagrut pratikjagrut changed the title [WIP] Fix random e2e failure. Fix random e2e failure. Jul 5, 2020
@pratikjagrut
Copy link
Contributor Author

/test 4.5-e2e
/test e2e

@pratikjagrut
Copy link
Contributor Author

/test 4.5-e2e

@DhritiShikhar
Copy link
Contributor

Tested this PR locally. E2E passes for me locally on 4.4 cluster.

@DhritiShikhar
Copy link
Contributor

/lgtm

@pratikjagrut
Copy link
Contributor Author

/test 4.5-e2e

@pratikjagrut
Copy link
Contributor Author

/test e2e

Avni-Sharma
Avni-Sharma reviewed Jul 6, 2020
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@pratikjagrut
Copy link
Contributor Author

/test e2e

@pratikjagrut
Copy link
Contributor Author

pratikjagrut commented Jul 8, 2020 •
edited

@Avni-Sharma I just tested e2e locally on 4.5 cluster they passed.

--- PASS: TestAddSchemesToFramework (573.91s)
    --- PASS: TestAddSchemesToFramework/end-to-end (401.57s)
        --- PASS: TestAddSchemesToFramework/end-to-end/scenario-etcd-unannotated-app-db-sbr (59.81s)
        --- PASS: TestAddSchemesToFramework/end-to-end/scenario-db-app-sbr (47.31s)
        --- PASS: TestAddSchemesToFramework/end-to-end/scenario-app-db-sbr (42.63s)
        --- PASS: TestAddSchemesToFramework/end-to-end/scenario-db-sbr-app (37.76s)
        --- PASS: TestAddSchemesToFramework/end-to-end/scenario-app-sbr-db (42.23s)
        --- PASS: TestAddSchemesToFramework/end-to-end/scenario-sbr-db-app (40.32s)
        --- PASS: TestAddSchemesToFramework/end-to-end/scenario-sbr-app-db (41.28s)
        --- PASS: TestAddSchemesToFramework/end-to-end/scenario-csv-db-app-sbr (40.96s)
        --- PASS: TestAddSchemesToFramework/end-to-end/scenario-csv-app-db-sbr (49.28s)
PASS

Anc they passed on CI too.

@Avni-Sharma
Copy link
Contributor

@Avni-Sharma I'm not sure what exactly happens on a local run to produce this error. Looks like secrets are empty, but on CI this error has not occurred yet.

Thanks @pratikjagrut . I will try running make test-e2e locally again for cluster 4.5 .

@Avni-Sharma
Copy link
Contributor

Passes on a fresh cluster for me now.

@Avni-Sharma
Copy link
Contributor

/lgtm

isutton
isutton suggested changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@isutton isutton left a comment

Choose a reason for hiding this comment

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

There are some changes that need to be performed in this PR still since some functions have lost their purpose since part of their business logic has been modified as noted in all inspect vs assert comments.

test/e2e/servicebindingrequest_test.go Outdated Show resolved Hide resolved
test/e2e/servicebindingrequest_test.go Outdated Show resolved Hide resolved
test/e2e/servicebindingrequest_test.go Outdated Show resolved Hide resolved
test/e2e/servicebindingrequest_test.go Outdated Show resolved Hide resolved
test/e2e/servicebindingrequest_test.go Outdated Show resolved Hide resolved
test/e2e/servicebindingrequest_test.go Outdated Show resolved Hide resolved
test/e2e/servicebindingrequest_test.go Outdated Show resolved Hide resolved
@pratikjagrut
Copy link
Contributor Author

/retest

@isutton
Copy link
Contributor

isutton commented Jul 8, 2020

@pratikjagrut any specific reason we're upgrading package in this PR ?

From the PR's description:

Update github.com/stretchr/testify from v1.4.0 to v1.6.1
The 1.4.0 version we were using had a bug in the Eventually function,
which caused flaky results.

@isutton
Copy link
Contributor

isutton commented Jul 8, 2020

/retest

@isutton
Copy link
Contributor

isutton commented Jul 8, 2020

/lgtm

@pratikjagrut
Fix random e2e failures.
b045cc6 
- Seed random with time.Now to generate unique digits on every subtest run.

- Replace retry code block with require.Eventually().

- Update github.com/stretchr/testify from v1.4.0 to v1.6.1
	The 1.4.0 version we were using had a bug in the Eventually function,
	which caused flaky results.

- Increasing timeout for existing Eventually function.

- Update vendor dependencies.

- Updating test timeout.

- Add --skip-cleanup-error=true flag.
@pratikjagrut
Copy link
Contributor Author

I pushed squashed commit.

@pmacik
Copy link
Contributor

pmacik commented Jul 8, 2020

Looks like the e2e tests failed for the infinite loop (#523)

@pmacik pmacik mentioned this pull request Jul 8, 2020
Closed
@pratikjagrut
Copy link
Contributor Author

/retest

@pratikjagrut
Copy link
Contributor Author

pratikjagrut commented Jul 9, 2020 •
edited

Looks like the e2e tests failed for the infinite loop (#523)

@pmacik One reason I could think of is if we create SBR before the application and then the operator is watching for the application deployment to be ready, and at the same time e2eutil.WaitForDeployment(t, f.KubeClient, ns, appName, 1, retryInterval, timeout) is also watching the same deployment. So here these two watches are in deadlock or sort of deadlock if operator watch detects the deployment is ready before e2eutil watch then it will redeploy it with the secrets hence the e2eutil watch might miss the first deployment readiness and will still wait for deployment this time deployment will take some more time to reach 1 replica ensuing e2eutil watch timeout. Hence failing the test.

@Avni-Sharma
Copy link
Contributor

Hi @pratikjagrut can you make an issue for the above-mentioned scenario so that we can look it up again.

@isutton
Copy link
Contributor

isutton commented Jul 9, 2020

/lgtm
/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: isutton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 97172e4 into redhat-developer:master Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akashshinde akashshinde akashshinde left review comments

@Avni-Sharma Avni-Sharma Avni-Sharma left review comments

@isutton isutton isutton approved these changes

@ldimaggi ldimaggi Awaiting requested review from ldimaggi

Assignees

@isutton isutton

@DhritiShikhar DhritiShikhar

@Avni-Sharma Avni-Sharma

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@pratikjagrut @DhritiShikhar @Avni-Sharma @isutton @pmacik @openshift-ci-robot @akashshinde @openshift-merge-robot