-
Notifications
You must be signed in to change notification settings - Fork 291
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
Bug Fix e2e tests #389
Bug Fix e2e tests #389
Conversation
c6b2e09
to
fca0df5
Compare
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 assume these changes are for further debugging of tests failure.
fca0df5
to
ca701de
Compare
@@ -104,11 +105,15 @@ func UpdateResource(t *testing.T, obj UpdateObjects) (*unstructured.Unstructured | |||
if err != nil { | |||
t.Fatalf("Failed to create %s: %v", obj.GVR.Resource, err) | |||
} | |||
// Provide Sufficient time interval for messages to reach fack servers | |||
time.Sleep(30 * time.Second) |
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.
Adding sleep is not reliable and cleaner way.
We should have a retry logic with timeout to wait till the condition met
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.
trying out possibilities to understand the problem with GitHub actions, since there is no issues with local testing.
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.
Now it is assured that it is due to lack of time for messages reaching fack servers, will work on logics and update here
yes @sanketsudake, The test cases are working fine in local, so trying to check through Github actions and debug further. |
2ad4433
to
522d347
Compare
This Commit, - Fixes build failure due to e2e tests - Fixes go vet suggestions - changes webook's payload mutex from RLock to Lock to prevent data loss - Add Mock time delays to allow propagation of events to all notifiers
522d347
to
87200d2
Compare
Tried different possible ways of fixing this issue. Doing so, we can understand that, we need to Mock the time delay involved in listening, filtering, and notifying events to all notifiers or we need to test each notifier individually verified the same with #268 as well, just by Mocking the event propagation time, this Issue is getting resolved @PrasadG193 @sanketsudake thoughts and comments on this..? |
@codenio let's merge this fix for now so that other PRs will get unblocked. We will definitely have to revisit this and come up with a better solution |
This Commit,
ISSUE TYPE
SUMMARY
Fixes #384