Skip to content

Commit

Permalink
Fix racy manager NotifiesBundleLoaded test (spiffe#4806)
Browse files Browse the repository at this point in the history
This test is racy because it currently relies on timing of several
goroutines and context cancellation.

The notifier doing context cancellation doesn't seem to test anything
useful. I don't recall why it was even added.

Getting rid of the context cancellation and simply asserting that the
function returns nil when the notifier returns nil seems good enough.

Signed-off-by: Andrew Harding <azdagron@gmail.com>
  • Loading branch information
azdagron authored and rushi47 committed Apr 11, 2024
1 parent 8f837e6 commit 2206c55
Showing 1 changed file with 6 additions and 16 deletions.
22 changes: 6 additions & 16 deletions pkg/server/ca/manager/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -722,31 +722,25 @@ func TestPruneCAJournals(t *testing.T) {
}

func TestRunNotifiesBundleLoaded(t *testing.T) {
test := setupTest(t)
test.initAndActivateSelfSignedManager(context.Background())

ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
defer cancel()
// time out in a minute if the bundle loaded never happens

test := setupTest(t)
test.initAndActivateSelfSignedManager(ctx)

var actual *common.Bundle
test.setNotifier(fakenotifier.New(t, fakenotifier.Config{
OnNotifyAndAdviseBundleLoaded: func(bundle *common.Bundle) error {
actual = bundle
cancel()
return nil
},
}))

// The bundle loaded handler above will cancel the
// context. This will usually be observed as an error returned from the
// notifier, but not always, depending on the timing of the cancellation.
// When the notifier does not return an error, Run will return without an
// error when the canceled context is passed internally to run the tasks.
err := test.m.NotifyBundleLoaded(ctx)
require.EqualError(t, err, "one or more notifiers returned an error: rpc error: code = Canceled desc = notifier(fake): context canceled")
require.NoError(t, err)

// make sure the event contained the bundle
expected := test.fetchBundle(context.Background())
expected := test.fetchBundle(ctx)
spiretest.RequireProtoEqual(t, expected, actual)
}

Expand All @@ -756,10 +750,6 @@ func TestRunFailsIfNotifierFails(t *testing.T) {

test := setupTest(t)
test.initAndActivateSelfSignedManager(ctx)
// manager, err := NewManager(ctx, test.selfSignedConfig())
// require.NoError(t, err)

// test.m = manager
test.setNotifier(fakenotifier.New(t, fakenotifier.Config{
OnNotifyAndAdviseBundleLoaded: func(bundle *common.Bundle) error {
return errors.New("ohno")
Expand Down

0 comments on commit 2206c55

Please sign in to comment.