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

Added Koperator e2e test - external consumer producer #991

Merged
merged 27 commits into from
Jun 22, 2023

Conversation

bartam1
Copy link
Contributor

@bartam1 bartam1 commented Jun 6, 2023

Description

Adding basic end to end tests for Koperator.
Included tests:

  • external kafka consumer producer
  • external kafka consumer producer SSL

Type of Change

  • Other (test)

Checklist

  • I have read the contributing guidelines
  • I have verified this change is not present in other open pull requests
  • All new and existing tests pass

@bartam1 bartam1 changed the base branch from master to test/de2e-koperator-uninstall June 6, 2023 09:42
@bartam1 bartam1 force-pushed the test/de2e-koperator-externalCP branch from efb3ee4 to 4cd451f Compare June 7, 2023 05:24
@bartam1 bartam1 marked this pull request as ready for review June 7, 2023 05:25
@bartam1 bartam1 requested a review from a team as a code owner June 7, 2023 05:25
Copy link
Contributor

@Kuvesz Kuvesz left a comment

Choose a reason for hiding this comment

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

Looks good really, couple of small nitpicks

// requireExternalProducerConsumer deploys a kafkaTopic into the K8s cluster
// and produces, consumes messages and makes comparison between the produced and consumed messages.
func requireExternalProducerConsumer(kubectlOptions *k8s.KubectlOptions) {
When("Externally produce and consume message to Kafka cluster", Ordered, func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When("Externally produce and consume message to Kafka cluster", Ordered, func() {
When("Externally produce and consume message to and from the Kafka cluster", Ordered, func() {


for i := range messages {
record := &kgo.Record{Topic: topicName, Value: []byte(messages[i])}
// Exists at the first error occurrence
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Exists at the first error occurrence
// Exits at the first error occurrence

Comment on lines 180 to 181
// This check may fail when the topic is re-created and has some undeleted content.
Expect(consumedMessages).Should(HaveLen(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this check in this case? We might end up reusing stuff in the end. Instead we could just compare with the last message from what we consumed here I think.

Copy link
Member

Choose a reason for hiding this comment

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

Will we actually run into this case that " the topic is re-created and has some undeleted content" in the test environment? If not, I think it's better to remove that comment to avoid confusions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed: 0e03f84

}

// requireExternalProducingConsumingMessage gets the Kafka cluster external addresses from the kafkaCluster CR status
// when externalAddresses is not specified and producing, consuming messages and make comparison between them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// when externalAddresses is not specified and producing, consuming messages and make comparison between them.
// when externalAddresses is not specified. It also produces and consumes messages and makes a comparison between them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed: 0e03f84

panyuenlau
panyuenlau previously approved these changes Jun 7, 2023
@bartam1 bartam1 force-pushed the test/de2e-koperator-uninstall branch from b905456 to 56b6b8a Compare June 14, 2023 15:21
@bartam1 bartam1 force-pushed the test/de2e-koperator-externalCP branch from 4cd451f to 3071ef6 Compare June 15, 2023 11:47
@bartam1 bartam1 force-pushed the test/de2e-koperator-uninstall branch from 1a732a8 to 46b2d71 Compare June 16, 2023 01:38
@bartam1 bartam1 force-pushed the test/de2e-koperator-externalCP branch from d029f87 to 0e03f84 Compare June 19, 2023 15:17
Base automatically changed from test/de2e-koperator-uninstall to master June 19, 2023 15:24
@bartam1 bartam1 dismissed panyuenlau’s stale review June 19, 2023 15:24

The base branch was changed.

pregnor
pregnor previously approved these changes Jun 20, 2023
tests/e2e/kafka_client.go Outdated Show resolved Hide resolved
tests/e2e/koperator_suite_test.go Show resolved Hide resolved
tests/e2e/produce_consume.go Outdated Show resolved Hide resolved
panyuenlau
panyuenlau previously approved these changes Jun 21, 2023
Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

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

looking sharp

@bartam1 bartam1 dismissed stale reviews from panyuenlau and pregnor via 61584fd June 22, 2023 04:12
Copy link
Contributor

@Kuvesz Kuvesz left a comment

Choose a reason for hiding this comment

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

Looks good!

kafkaTopicTemplate,
map[string]interface{}{
"Name": topicName,
"TopicName": topicName,
"Namespace": kubectlOptions.Namespace,
},
)
err := waitK8sResourceCondition(kubectlOptions, kafkaTopicKind, "jsonpath={.status.state}=created", defaultTopicCreationWaitTime, "", topicName)
Expect(err).ShouldNot(HaveOccurred())
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think we should refactor these later on to be the same across our code base, but I don't think this should block merging now.

@bartam1 bartam1 merged commit db704f0 into master Jun 22, 2023
@bartam1 bartam1 deleted the test/de2e-koperator-externalCP branch June 22, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants