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

reducing amount of "eventually"s in e2e tests #100

Merged

Conversation

RamLavi
Copy link
Member

@RamLavi RamLavi commented Mar 4, 2020

Most of the kubemacpool tests are using 'Eventually' method.
Using this method may hide bugs (for example).

In this PR we will reexamine the use of eventually in the e2e tests, to achieve the following goal:

  1. remove eventually when is not needed.
  2. in places where eventually is needed, strive to make it as specific as possible by adding "Expect" method for the specific error we are expecting to get while failing.

As part of opening this PR, we will signal as 'P' tests that show unstable behavior as a result of the removal of eventually - indicating a potential bug. For each there will be opened a corresponding issue.

Signed-off-by: Ram Lavi [email protected]

Copy link
Contributor

@alonSadan alonSadan left a comment

Choose a reason for hiding this comment

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

just small initial review. not really contributed to the discussion about the eventually for now.

tests/virtual_machines_test.go Show resolved Hide resolved
tests/virtual_machines_test.go Show resolved Hide resolved
@alonSadan
Copy link
Contributor

@phoracek @RamLavi
notes are here :
#106

@RamLavi RamLavi force-pushed the drop_eventually_in_tests branch 2 times, most recently from ece37b8 to 974d0db Compare March 26, 2020 12:35
@RamLavi RamLavi changed the title WIP reducing amount of "eventually"s in e2e tests reducing amount of "eventually"s in e2e tests Mar 26, 2020
@RamLavi RamLavi force-pushed the drop_eventually_in_tests branch 3 times, most recently from 17399ef to 8adf567 Compare March 26, 2020 16:24
Copy link
Member

@phoracek phoracek left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

As long as we fix all the skipped tests after the refactoring.

@phoracek
Copy link
Member

@RamLavi feel free to cancel the hold

@RamLavi
Copy link
Member Author

RamLavi commented Mar 27, 2020

/lgtm
/approve
/unhold

@kubevirt-bot
Copy link
Collaborator

@RamLavi: you cannot LGTM your own PR.

In response to this:

/lgtm
/approve
/unhold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phoracek, RamLavi

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

@phoracek
Copy link
Member

/lgtm

@phoracek
Copy link
Member

/release-note-none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants