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

Fix TestCheckKafkaTopicExist #943

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Fix TestCheckKafkaTopicExist #943

merged 1 commit into from
Mar 6, 2023

Conversation

bartam1
Copy link
Contributor

@bartam1 bartam1 commented Mar 3, 2023

Q A
Bug fix? yes
New feature? no
API breaks? no
Deprecations? no
License Apache 2.0

What's in this PR?

Fix test for that case when the Kafka topic exist on the Kafka cluster and we want to create KafkaTopic Cr for that.

Why?

The current implementation only checks the last case from the test cases and there is a wrong test.

Checklist

  • Implementation tested

@bartam1 bartam1 requested a review from a team as a code owner March 3, 2023 11:51
@@ -193,6 +193,7 @@ func TestCheckKafkaTopicExist(t *testing.T) {
}

for _, testCase := range testCases {
testCase := testCase
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member

@pregnor pregnor Mar 3, 2023

Choose a reason for hiding this comment

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

Because of how the iterator works in Go together with the capture variables for the lambda function defined on the next line where the iiterator is a lambda capture variable and running the test subcases in a parallel fashion.

With t.Parallel() the iteration just fires the subtests but won't be blocked until it finishes, so multiple t.Run()s are going to be executed at the same time.
All of those are going to refer to &testCase.KafkaTopic for example and when the iteration moves ahead the testCase reference (it is a special pseudo-pointer, IDK how to describe it better than the C++ term) will be updated in place to the new value.
If the iteration is at test case X and executes t.Run() then detaches the thread with t.Parallel() and the iteration moves ahead to test case Y updating the testCase iterator then X's thread executes the the fieldErrorList, err := kafkaTopicValidator.checkKafka(context.Background(), &testCase.kafkaTopic, cluster) statement, it will evaluate &testCase.kafkaTopic to Y's testCase.kafkaTopic value instead of X's.
By declaring another variable inside the iteration to hold the iterator's value, we copy the original content, ensuring the lambda function always calls the iteration-local copy which means using X's values in X's lambda function.
The convention of naming the iteration local variable the same way as the iterator is to ease the developer's work because semantically the iterator changing and the lambda function using the new value of it is merely a Go optimization internal detail the developer doesn't care much about.

Edit: iteration pointer->iterator (but it is important this is a reference/pseudo-pointer semantically from iteration advancement standpoint in Go)

Copy link
Contributor

Choose a reason for hiding this comment

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

I had no idea, thanks a lot for explaining!

@bartam1 bartam1 merged commit 2283f93 into master Mar 6, 2023
@bartam1 bartam1 deleted the fix_ktopic_valid_test branch March 6, 2023 10:44
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.

3 participants