-
Notifications
You must be signed in to change notification settings - Fork 542
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
Replace assertions in Catalog-e2e to use Gomega's Matcher library #1502
Replace assertions in Catalog-e2e to use Gomega's Matcher library #1502
Conversation
8d5c0b2
to
dd6bc94
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit, not a necessary change. /lgtm
/retest |
cfff446
to
38ccdb2
Compare
/test e2e-aws-olm |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
test/e2e/catalog_e2e_test.go
Outdated
|
||
// Ensure the timing | ||
catalogConnState := fetchedUpdatedCatalog.Status.GRPCConnectionState | ||
subUpdatedTime := subscription.Status.LastUpdated | ||
timeLapse := subUpdatedTime.Sub(catalogConnState.LastConnectTime.Time).Seconds() | ||
require.True(GinkgoT(), timeLapse < 60) | ||
Expect(timeLapse).Should(BeNumerically("<", 60)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking nit: There's a built-in Gomega matcher for time.Time
: https://pkg.go.dev/github.com/onsi/gomega?tab=doc#BeTemporally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, I don't see if it could be used here since we are asserting that the var timeLapse
has a value < 60. BeTemporarily
expects 60 to be of type time.Time{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Ben is suggesting something like Expect(catalogConnState.LastConnectTime.Time).Should(BeTemporally("<", subUpdatedTime.Add(60 * time.Second)
@harishsurf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it to:
Expect(subUpdatedTime.Time).Should(BeTemporally("<", catalogConnState.LastConnectTime.Add(60*time.Second)))
if err != nil { | ||
return err | ||
} | ||
Eventually(waitForScaleup, 5*time.Minute, 1*time.Second).Should(BeTrue()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't 5 minutes and 1 second the defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default timeout is 1min
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, harishsurf 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 |
38ccdb2
to
dcf9d6d
Compare
/retest |
2 similar comments
/retest |
/retest |
@harishsurf: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
/test e2e-upgrade |
dcf9d6d
to
c09facf
Compare
This PR failed 1 out of 1 times with 4 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits. totaltestcount: 1
|
/lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
Description of the change:
This PR replaces
testify/require
assertions with Gomega's assertions. This PR will be followed up with another PR that uses ginkgo'sContainer
blocks to ensure BDD style structuringMotivation for the change:
Reviewer Checklist
/docs