Skip to content

test: Add t.Cleanup() to fix TestEventHandler #60203

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+4 −0
Open

test: Add t.Cleanup() to fix TestEventHandler #60203

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions integrations/event-handler/event_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ func (s *EventHandlerSuite) startApp() {
app, err := NewApp(&s.appConfig, slog.Default())
require.NoError(t, err)

t.Cleanup(func() {
t.Log("Close the app...")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this log line to reduce noise.

Suggested change
t.Log("Close the app...")

Copy link
Author

Choose a reason for hiding this comment

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

Hi, thanks for the comment. I will remove the log line to reduce the noise.
I have found a similar issue (#13501), which was fixed in PR(#40253) by disabling the services that write to disk.
Given the similar issue and corresponding solution, I attempted to replace the AuditLog and Streamer with discard versions.
Before:

func TestEventHandler(t *testing.T) {
	suite.Run(t, &EventHandlerSuite{
		AuthHelper: &integration.MinimalAuthHelper{},
	})
}

After:

func TestEventHandler(t *testing.T) {
	suite.Run(t, &EventHandlerSuite{
		AuthHelper: &integration.MinimalAuthHelper{
            // Add AuditLog and Streamer with discard version
			AuthConfig: authtest.AuthServerConfig{
				AuditLog: events.NewDiscardAuditLog(),
				Streamer: events.NewDiscardStreamer(), 
			},
		},
	})
}

However, it introduced context deadline exceeded timeout error because the test requires AuditLog to provide events in TestEvent, and the AuditLog cannot be discarded.

This leads me to my question: my goal is to replace the file-based AuditLog with in-memory version for this test. This would solve the race condition without breaking the test's logic. However, I couldn't find the in-memory AuditLog in the codebase (Please correct me if I am wrong).

Am I on the right track with this in-memory approach? Any guidance on how to set up an in-memory AuditLog for this test would be appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The eventstest package has some helpers though I don't know that a full in memory implementation exists.

Am I on the right track with this in-memory approach?

Have you confirmed that the directory that is still being written to on shutdown is the one used for the audit log? Instead of writing an entirely new audit log implementation, can we ensure that no more audit events are written to disk during shutdown?

app.Close()
})
integration.RunAndWaitReady(s.T(), app)
}

Expand Down
Loading