tigerquoll commented on code in PR #1074:
URL: https://github.com/apache/yunikorn-k8shim/pull/1074#discussion_r3857339004
##########
pkg/shim/scheduler_test.go:
##########
@@ -175,9 +175,53 @@ func TestSchedulerRegistrationFailed(t *testing.T) {
ctx := cache.NewContext(mockedAPIProvider)
shim := newShimSchedulerInternal(ctx, mockedAPIProvider, callback)
assert.Error(t, shim.Run(), "some error")
+ assertStopChannelClosed(t, shim)
+
+ // Stop must remain safe when Run() already stopped the shim after its
failure.
+ shim.Stop()
+}
+
+func TestSchedulerStopClosesStopChannel(t *testing.T) {
+ mockedAPIProvider := client.NewMockedAPIProvider(false)
+ shim := newShimSchedulerInternal(cache.NewContext(mockedAPIProvider),
mockedAPIProvider, nil)
+
+ shim.Stop()
+ assertStopChannelClosed(t, shim)
+
+ // Stop must be idempotent because callers can invoke it after a failed
Run().
+ shim.Stop()
+}
+
+func TestSchedulerStopStopsAPIFactory(t *testing.T) {
Review Comment:
Minor: this and `TestSchedulerStopClosesStopChannel` are the same test with
one assertion swapped — `Stop()` closes the channel and stops the API factory
in the same `Once` body, so a single `TestSchedulerStop` can `assert.Check`
both after one `Stop()` call.
Also, `trackingAPIProvider` re-implements what `MockedAPIProvider` already
tracks in its `running` field (cleared in `Stop()`). An `IsRunning()` getter on
the mock would let this test drop the wrapper — note the test would need to
call `RunEventHandler()` first, otherwise `running` is never set and the
assertion is vacuous.
##########
pkg/shim/scheduler_test.go:
##########
@@ -175,9 +175,53 @@ func TestSchedulerRegistrationFailed(t *testing.T) {
ctx := cache.NewContext(mockedAPIProvider)
shim := newShimSchedulerInternal(ctx, mockedAPIProvider, callback)
assert.Error(t, shim.Run(), "some error")
+ assertStopChannelClosed(t, shim)
+
+ // Stop must remain safe when Run() already stopped the shim after its
failure.
+ shim.Stop()
+}
+
+func TestSchedulerStopClosesStopChannel(t *testing.T) {
Review Comment:
None of the new tests actually reach the defect this PR fixes. The 3367 bug
is "one send stops only one of the two `wait.Until` loops", but:
- `TestSchedulerRegistrationFailed` fails inside `registerShimLayer()`
before `doScheduling()` is ever called
- `TestSchedulerStopClosesStopChannel` and
`TestSchedulerStopStopsAPIFactory` never call `Run()`
So the loops don't exist in any of them, and `assertStopChannelClosed` is a
non-blocking receive that a regression back to a buffered single send would
also pass. The `MockScheduler` tests do run `Run()`+`Stop()` but assert nothing
about the goroutines exiting.
Could we add one test that runs `Run()` successfully against the mock, calls
`Stop()`, and asserts both loop goroutines terminate? `goleak.VerifyNone`
scoped to that test, or a small counter/WaitGroup on the shim that the test can
wait on, would both work. That's the regression guard for the actual fix.
##########
pkg/shim/scheduler_mock_test.go:
##########
@@ -204,7 +204,6 @@ func (fc *MockScheduler) waitAndVerifySchedulerAllocations(
func (fc *MockScheduler) stop() {
close(fc.stopChan)
fc.scheduler.Stop()
Review Comment:
Removing this call fixes the double close, but it does so by coupling the
harness to the shim's internal call order. `MockedAPIProvider.Stop()` already
takes the lock and has a `running` flag, so guarding it with `if !m.running {
return }` makes it safe for any caller and would let this line stay. Either is
fine for this PR; the guard is more robust for future tests that build their
own provider.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
