-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Introduce test.NewTempWindow() to avoid memory leak in test windows #4849
base: develop
Are you sure you want to change the base?
Conversation
I don't think we should deprecate test.NewWindow as it is a legitimate thing to do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a great change, but a couple of questions inline from reading it through.
test/testwindow.go
Outdated
// | ||
// Since: 2.5 | ||
func NewTempWindow(t testing.TB, content fyne.CanvasObject) fyne.Window { | ||
window := fyne.CurrentApp().NewWindow("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be test.NewWindow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Should be fixed now
@@ -2142,11 +2112,6 @@ func setupSelection(t *testing.T, reverse bool) (*widget.Entry, fyne.Window) { | |||
return e, window | |||
} | |||
|
|||
func teardownImageTest(w fyne.Window) { | |||
w.Close() | |||
test.NewApp() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is no longer called from many tests. Is that ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is expected. I wired up setupImageTest
and setupPasswordTest
to both use NewTempWindow
and create a new application when cleaning up after the test.
Description:
I noticed that we had a quite substantial memory leak in our test suite due to various tests forgetting to call .Close() on the test window that was created. This led to larger memory usage than what would be ideal. I fixed all the issues I could find and introduced a
test.NewTempWindow()
function to create a window that automatically closes at the end of the test.Any places where we forgot to call Close() now uses the new function. Old uses that close the window have been left as is. Should we deprecate the old function to point people towards this safer new function?
Checklist:
Where applicable: