Added tests for Cloud Events and verification of reception of events by the Sink #93
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kyma-bot
added
the
size/XL
muralov
reviewed
Sep 13, 2023
There was a problem hiding this comment.
I have found some other improvements not specific to the PR:
- I haven't seen any clean-up for eventing-manager in cleanup_test go files. Do we delete it at the endo file.
- We need to correct the comment
- We need to join errors in test cases so that we show all differences so that we see all at once. You can use errrors.Join
Comment on lines
438
to
473
| func (te *TestEnvironment) CompareCloudEvents(expectedEvent cloudevents.Event, gotEvent cloudevents.Event) error { | ||
| // check if its a valid CloudEvent. | ||
| if err := gotEvent.Validate(); err != nil { | ||
| return fmt.Errorf("expected valid cloud event, but got invalid cloud event. Error: %s", err.Error()) | ||
| } | ||
|
|
||
| if expectedEvent.ID() != gotEvent.ID() { | ||
| return fmt.Errorf("expected event ID: %s, got event ID: %s", expectedEvent.ID(), gotEvent.ID()) | ||
| } | ||
|
|
||
| if string(expectedEvent.Data()) != string(gotEvent.Data()) { | ||
| return fmt.Errorf("expected event data: %s, got event data: %s", | ||
| string(expectedEvent.Data()), string(gotEvent.Data())) | ||
| } | ||
|
|
||
| // if it is a v1alpha1 Subscription event, then we do not check further. | ||
| if strings.HasPrefix(gotEvent.Type(), te.TestConfigs.EventTypePrefix) { | ||
| return nil | ||
| } | ||
|
|
||
| // check in detail further the source and type. | ||
| if expectedEvent.Source() != gotEvent.Source() { | ||
| return fmt.Errorf("expected event Source: %s, got event Source: %s", expectedEvent.Source(), gotEvent.Source()) | ||
| } | ||
|
|
||
| if expectedEvent.Type() != gotEvent.Type() { | ||
| return fmt.Errorf("expected event Type: %s, got event Type: %s", expectedEvent.Type(), gotEvent.Type()) | ||
| } | ||
|
|
||
| originalType, ok := gotEvent.Extensions()[fixtures.EventOriginalTypeHeader] | ||
| if !ok { | ||
| return fmt.Errorf("expected event to have header: %s, but its missing", fixtures.EventOriginalTypeHeader) | ||
| } | ||
| if expectedEvent.Type() != originalType { | ||
| return fmt.Errorf("expected originaltype header to have value: %s, but got: %s", expectedEvent.Type(), originalType) | ||
| } |
There was a problem hiding this comment.
It'd be nice if we can collect all the errors wiht errors.Join so that we can see at once what are all differences. Otherwise, it returns and the following differences are not shown.
| func (te *TestEnvironment) VerifyCloudEventReceivedBySink(expectedEvent cloudevents.Event) error { | ||
| // define the event | ||
| gotSinkEvent, err := te.SinkClient.GetEventFromSinkWithRetries(expectedEvent.ID(), Attempts, Interval) | ||
| if err != nil { | ||
| te.Logger.Debug(err.Error()) |
There was a problem hiding this comment.
is this error not displayed twice in the log as error is returned to the main test case? You should have seen the logs.
There was a problem hiding this comment.
It is printed here so we can see the progress during the multiple retries.
There was a problem hiding this comment.
Can we make a common function to pass message, subscriptions, CRD version (v1alpha1/2) and CE encoding. Let's keep test cases as they are, but to extract to a common function for iterating subscriptions and testing them. It seems to me lots of repetitive code.
For example,
function testEventDelivery(subsctipions, encoding, crdVersion) {
for _, subToTest := range subscriptions {
subToTest := subToTest
for _, eventTypeToTest := range subToTest.Types {
eventTypeToTest := eventTypeToTest
testName := fmt.Sprintf("legacy event should work for subscription: %s with type: %s", subToTest.Name, eventTypeToTest)
// run test for the eventType.
t.Run(testName, func(t *testing.T) {
t.Parallel()
// when
err := common.Retry(testenvironment.ThreeAttempts, testenvironment.Interval, func() error {
return testEnvironment.TestDeliveryOfLegacyEvent("", eventTypeToTest, crdVersion, encoding)
})
// then
require.NoError(t, err)
})
}
}
}
muralov
reviewed
Sep 13, 2023
There was a problem hiding this comment.
I have found some other improvements not specific to the PR:
- I haven't seen any clean-up for eventing-manager in cleanup_test go files. Do we delete it e2e/cleanup/cleanup_test.go?
- We need to correct the comment
- We need to join errors in test cases so that we show all differences so that we see all at once. You can use errors.Join
- Are the errors are not displayed twice? Let's run them together and check how to it looks.
We are not deploying |
muralov
approved these changes
Sep 14, 2023
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Changes proposed in this pull request:
Related issue(s)