feat: Tests for IMC controller join/leave flow.

[minhng22]

Description of your changes

Tests for IMC controller join/leave flow.

I have:

• Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Coverage of test suite is 95.8% on member_controller.go

Special notes for your reviewer

[codecov]

Codecov upload limit reached ⚠️

This org is currently on the free Basic Plan; which includes 250 free private repo uploads each rolling month. This limit has been reached and additional reports cannot be generated. For unlimited uploads, upgrade to our pro plan.

Do you have questions or need help? Connect with our sales team today at sales@codecov.io

[minhng22]

Fixing the data race issue: for the negative test cases(specs), there was no message sending to the imcChan (because the reconciliation doesn't succeed).

Since chan blocks goroutine by default until both reader/ sender goroutine is ready (tour of Go), the goroutine that reads the chan causes data race when the imcchan var is reassigned in the next test.

Close channel won't solve the problem as when the chan is reassigned, the goroutine picks up, as tested here: https://go.dev/play/p/kjKINIrdYak

Solution is to have a sender for each reader of the chan and the other way around, as tested here: https://go.dev/play/p/8r8vTLexxRq

So sending some message to imc chan fixes the issue.

Some useful short readings:

• https://mipsmonsta.medium.com/understanding-blocking-nature-of-golang-channel-2c1c95bf5d82
• https://medium.com/rungo/anatomy-of-goroutines-in-go-concurrency-in-go-a4cb9272ff88#:~:text=goroutines%20always%20run%20in%20the,value%20from%20goroutine%20is%20ignored.
• https://medium.com/rungo/anatomy-of-channels-in-go-concurrency-in-go-1ec336086adb

[minhng22]

The last commit is actually where I implemented the fix, although its name is make reviewable.

[ryanzhang-oss]

Fixing the data race issue: for the negative test cases(specs), there was no message sending to the imcChan (because the reconciliation doesn't succeed).

Since chan blocks goroutine by default until both reader/ sender goroutine is ready (tour of Go), the goroutine that reads the chan causes data race when the imcchan var is reassigned in the next test.

Close channel won't solve the problem as when the chan is reassigned, the goroutine picks up, as tested here: https://go.dev/play/p/kjKINIrdYak

Solution is to have a sender for each reader of the chan and the other way around, as tested here: https://go.dev/play/p/8r8vTLexxRq

So sending some message to imc chan fixes the issue.

Some useful short readings:

• https://mipsmonsta.medium.com/understanding-blocking-nature-of-golang-channel-2c1c95bf5d82
• https://medium.com/rungo/anatomy-of-goroutines-in-go-concurrency-in-go-a4cb9272ff88#:~:text=goroutines%20always%20run%20in%20the,value%20from%20goroutine%20is%20ignored.
• https://medium.com/rungo/anatomy-of-channels-in-go-concurrency-in-go-1ec336086adb

Good learning! I don't really understand what the data race is though as the channel is closed at the end of each test. The only problem I see is that the channel is shared between two tests. Just separating them out should remove the so-called "race".

[minhng22]

I don't really understand what the data race is though as the channel is closed at the end of each test.

-> This is where I scratched my head too as I thought the goroutine reading the chan should be closed (and never reopen) when the chan is closed. But now I think when the chan is closed then chan var is reassigned, the go routine that previously read the channel (BUT never receive any message) came alive again. I tested that behavior here: https://go.dev/play/p/8WXRykgyxPN

If I sent some message to the chan the the behavior works as expected, as tested here: https://go.dev/play/p/kjKINIrdYak

The only problem I see is that the channel is shared between two tests. Just separating them out should remove the so-called "race".

-> I agreed this setup ends up less straight forward than I thought so I don't really like it either. However I invested quite some time trying to create a completely new chan per test and it seems to be even messier as we needs to somehow setup the chan in BeforeEach but close in AfterEach without using a global variable like current setup. If there is a cleaner way I am down for changing.

[Arvindthiru]

Clarification: Why are we sending the state as Leave here for the Join test?

[minhng22]

Short answer: We need to balance reader and writer goroutine for imc chan. This is after the test is done so what we send doesn't affect the test. I could create a nested context and put this in AfterEach but Ryan feedback in my last PR of a similar usage that it's not necessary.

Long answer: Details above of the error that happens when there is reader goroutine but no writer and vice versa.

[Arvindthiru]

Hey @minhng22 I don't think this part got tested in the Join workflow https://github.com/minhng22/fleet/blob/imc-controller-1/pkg/controllers/internalmembercluster/member_controller.go#L96-L99. Open question @ryanzhang-oss @helayoty are we expected to test this here since there are no unit tests to cover this as well.

[minhng22]

Hey @minhng22 I don't think this part got tested in the Join workflow https://github.com/minhng22/fleet/blob/imc-controller-1/pkg/controllers/internalmembercluster/member_controller.go#L96-L99. Open question @ryanzhang-oss @helayoty are we expected to test this here since there are no unit tests to cover this as well.

I am curious to know what other thinks. I don't include a test case for it in this PR to keep this PR smaller and be reviewed faster( but it seems like that doesn't help much). I could add a test case to cover that but I am not sure it's needed since the coverage is already above 90% while we decided 80% is enough.

[minhng22]

Hey @minhng22 I don't think this part got tested in the Join workflow https://github.com/minhng22/fleet/blob/imc-controller-1/pkg/controllers/internalmembercluster/member_controller.go#L96-L99. Open question @ryanzhang-oss @helayoty are we expected to test this here since there are no unit tests to cover this as well.

I am curious to know what other thinks. I don't include a test case for it in this PR to keep this PR smaller and be reviewed faster( but it seems like that doesn't help much). I could add a test case to cover that but I am not sure it's needed since the coverage is already above 90% while we decided 80% is enough.

My preference is we close this PR regardless of whether we should add a test case for that, since this PR is getting too big, which makes it susceptible to more mistakes. Also the coverage is already higher than enough.

[helayoty]

My preference is we close this PR regardless of whether we should add a test case for that, since this PR is getting too big, which makes it susceptible to more mistakes. Also the coverage is already higher than enough.

Yes please, open a new PR.