Skip to content
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

That's the races out of our test suite! #5384

Open
wants to merge 43 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
8412c9d
Synchronise the theme watcher and other "background" items in test code
andydotxyz Jan 7, 2025
d35da3e
Refactored test package to not have races in the app settings applica…
andydotxyz Jan 8, 2025
5ae7d16
Resolve a race in the preference code
andydotxyz Jan 9, 2025
d28e1c5
Remove lock complications in test, they now behave better
andydotxyz Jan 9, 2025
145dd07
Correct race in mobile double tap handling
andydotxyz Jan 9, 2025
89f5f20
Remove locks and fix thread safety of cursor animation
andydotxyz Jan 9, 2025
b0e5c64
Move the window safety code into a wrapper type
andydotxyz Jan 9, 2025
08d798a
Fixes to canvas tests and what looked like 2 bad test files
andydotxyz Jan 9, 2025
00d741f
The final runOnMains for testing, and enable race checks in CI!
andydotxyz Jan 9, 2025
4c0458c
Merge branch 'develop' into fix/racedetection
andydotxyz Jan 9, 2025
7d5fb07
Fix merge conflict
andydotxyz Jan 9, 2025
367a6a6
Always use safe canvas in tests
andydotxyz Jan 9, 2025
895953c
Fix race in animation tests
andydotxyz Jan 9, 2025
13d1f77
Don't use goroutines in preference applciation
andydotxyz Jan 9, 2025
4713739
Add race to mobile and wayland
andydotxyz Jan 9, 2025
a0ecb94
Fixing some more window test races
andydotxyz Jan 9, 2025
98dc17d
Missed another goroutine
andydotxyz Jan 9, 2025
b97878b
Remove test that doesn't agree with pool contract
andydotxyz Jan 9, 2025
f3fbe55
Ensure it's not a race to start tests that is tripping mobile
andydotxyz Jan 9, 2025
e05f5ed
safety in window capture too
andydotxyz Jan 9, 2025
05919eb
More safety in tests
andydotxyz Jan 9, 2025
06b616b
Correcting test harness to fully run mobile and fix threading of doub…
andydotxyz Jan 10, 2025
d4f57b7
Be consistent with GLFW and wait for the main request
andydotxyz Jan 10, 2025
6d8675d
Attempting full mobile run
andydotxyz Jan 10, 2025
ddacc2c
Merge branch 'develop' into fix/racedetection
andydotxyz Jan 10, 2025
fb997d8
More safety in preference write
andydotxyz Jan 10, 2025
afb5db5
Lighter weight mobile test - more than before this PR but not the ful…
andydotxyz Jan 10, 2025
52a15d9
Potential issue with preferences and mobile
andydotxyz Jan 10, 2025
a446af3
Adjusting thread on file watcher callbacks
andydotxyz Jan 10, 2025
154d1e6
Fix staticcheck
andydotxyz Jan 10, 2025
c2f8169
More GLFW fixes
andydotxyz Jan 10, 2025
2b9f46d
Correct threading of cloud apply test
andydotxyz Jan 10, 2025
b37b4a5
Don't "find" app files for dialog tests
andydotxyz Jan 10, 2025
df498a6
Fix a race in menu bar tests on Ubuntu
andydotxyz Jan 10, 2025
49cbf18
Menu item safety for Ubuntu tests too
andydotxyz Jan 10, 2025
190d747
GLFW tests need real main thread
andydotxyz Jan 10, 2025
f04b0c9
Remove last worrying goroutines
andydotxyz Jan 10, 2025
6cec612
Same fix avoiding import loop
andydotxyz Jan 10, 2025
1ae8045
Safer tapping around in GLFW tests
andydotxyz Jan 10, 2025
bcb6742
Protecting more complex tap flows
andydotxyz Jan 10, 2025
15b454e
Passing locally on Linux now too
andydotxyz Jan 10, 2025
7dbcac0
Fixing some intermittent
andydotxyz Jan 10, 2025
62d9f9a
Fixing another occasional race
andydotxyz Jan 10, 2025
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
5 changes: 4 additions & 1 deletion .github/workflows/mobile_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ jobs:
fail-fast: false
matrix:
go-version: ['1.19.x', '1.23.x']
include:
- os: ubuntu-latest
runner: xvfb-run

steps:
- uses: actions/checkout@v4
Expand All @@ -23,4 +26,4 @@ jobs:
run: sudo apt-get update && sudo apt-get install gcc libegl1-mesa-dev libgles2-mesa-dev libx11-dev xorg-dev

- name: Tests
run: go test -test.benchtime 10ms -tags "ci mobile" ./...
run: go test -race -test.benchtime 10ms -tags "ci mobile" ./...
6 changes: 3 additions & 3 deletions .github/workflows/platform_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ jobs:
if: ${{ runner.os == 'Linux' }}

- name: Tests
run: ${{ matrix.runner }} go test "-test.benchtime" 10ms -tags "${{ matrix.tags }}" ./...
run: ${{ matrix.runner }} go test -race "-test.benchtime" 10ms -tags "${{ matrix.tags }}" ./...

- name: Wayland Tests
run: go test -tags no_glfw,ci,wayland ./...
run: go test -race -tags no_glfw,ci,wayland ./...
if: ${{ runner.os == 'Linux' }}

windows_tests:
Expand All @@ -61,4 +61,4 @@ jobs:
go-version: ${{ matrix.go-version }}

- name: Tests
run: ${{ matrix.runner }} go test "-test.benchtime" 10ms -tags no_glfw ./...
run: ${{ matrix.runner }} go test -race "-test.benchtime" 10ms -tags no_glfw ./...
2 changes: 1 addition & 1 deletion app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (a *fyneApp) NewWindow(title string) fyne.Window {
}

func (a *fyneApp) Run() {
go a.lifecycle.RunEventQueue()
go a.lifecycle.RunEventQueue(a.driver.CallFromGoroutine)
a.driver.Run()
}

Expand Down
23 changes: 15 additions & 8 deletions app/cloud_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@ import (
"fyne.io/fyne/v2"
"fyne.io/fyne/v2/internal"
"fyne.io/fyne/v2/storage"
"fyne.io/fyne/v2/test"
"fyne.io/fyne/v2/theme"

"github.com/stretchr/testify/assert"
)

func TestFyneApp_SetCloudProvider(t *testing.T) {
a := NewWithID("io.fyne.test")
a := test.NewApp()
p := &mockCloud{}
a.SetCloudProvider(p)

Expand All @@ -22,7 +23,7 @@ func TestFyneApp_SetCloudProvider(t *testing.T) {
}

func TestFyneApp_SetCloudProvider_Cleanup(t *testing.T) {
a := NewWithID("io.fyne.test")
a := test.NewApp()
p1 := &mockCloud{}
p2 := &mockCloud{}
a.SetCloudProvider(p1)
Expand All @@ -37,22 +38,28 @@ func TestFyneApp_SetCloudProvider_Cleanup(t *testing.T) {
}

func TestFyneApp_transitionCloud(t *testing.T) {
a := NewWithID("io.fyne.test")
a := test.NewApp()
p := &mockCloud{}
preferenceChanged := false
settingsChan := make(chan fyne.Settings)
a.Preferences().AddChangeListener(func() {
preferenceChanged = true
})
a.Settings().AddChangeListener(settingsChan)
a.SetCloudProvider(p)

<-settingsChan // settings were updated
assert.True(t, preferenceChanged)
done := make(chan struct{})
go func() {
<-settingsChan // settings were updated
assert.True(t, preferenceChanged)
close(done)
}()

a.SetCloudProvider(p)
<-done
}

func TestFyneApp_transitionCloud_Preferences(t *testing.T) {
a := NewWithID("io.fyne.test")
a := test.NewApp()
a.Preferences().SetString("key", "blank")

assert.Equal(t, "blank", a.Preferences().String("key"))
Expand All @@ -64,7 +71,7 @@ func TestFyneApp_transitionCloud_Preferences(t *testing.T) {
}

func TestFyneApp_transitionCloud_Storage(t *testing.T) {
a := NewWithID("io.fyne.test")
a := test.NewApp()
a.Storage().Create("nothere")

l := a.Storage().List()
Expand Down
20 changes: 12 additions & 8 deletions app/preferences.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,19 @@ func (p *preferences) forceImmediateSave() {
func (p *preferences) resetSavedRecently() {
go func() {
time.Sleep(time.Millisecond * 100) // writes are not always atomic. 10ms worked, 100 is safer.
p.prefLock.Lock()
p.savedRecently = false
changedDuringSaving := p.changedDuringSaving
p.changedDuringSaving = false
p.prefLock.Unlock()

if changedDuringSaving {
p.save()
}
// For test reasons we need to use current app not what we were initialised with as they can differ
fyne.CurrentApp().Driver().CallFromGoroutine(func() {
p.prefLock.Lock()
p.savedRecently = false
changedDuringSaving := p.changedDuringSaving
p.changedDuringSaving = false
p.prefLock.Unlock()

if changedDuringSaving {
p.save()
}
})
}()
}

Expand Down
3 changes: 2 additions & 1 deletion app/preferences_other.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package app
import (
"path/filepath"

"fyne.io/fyne/v2"
"fyne.io/fyne/v2/internal/app"
)

Expand All @@ -27,6 +28,6 @@ func (p *preferences) watch() {
return
}

p.load()
fyne.CurrentApp().Driver().CallFromGoroutine(p.load)
})
}
13 changes: 11 additions & 2 deletions data/binding/pref_helper.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package binding

import (
"sync"

"fyne.io/fyne/v2"
"fyne.io/fyne/v2/internal/async"
)
Expand All @@ -26,6 +28,7 @@ type preferencesMap struct {
prefs async.Map[fyne.Preferences, *preferenceBindings]

appPrefs fyne.Preferences // the main application prefs, to check if it changed...
appLock sync.Mutex
}

func newPreferencesMap() *preferencesMap {
Expand All @@ -44,10 +47,14 @@ func (m *preferencesMap) ensurePreferencesAttached(p fyne.Preferences) *preferen

func (m *preferencesMap) getBindings(p fyne.Preferences) *preferenceBindings {
if p == fyne.CurrentApp().Preferences() {
m.appLock.Lock()
prefs := m.appPrefs
if m.appPrefs == nil {
m.appPrefs = p
} else if m.appPrefs != p {
m.migratePreferences(m.appPrefs, p)
}
m.appLock.Unlock()
if prefs != p {
m.migratePreferences(prefs, p)
}
}
binds, _ := m.prefs.Load(p)
Expand All @@ -72,7 +79,9 @@ func (m *preferencesMap) migratePreferences(src, dst fyne.Preferences) {

m.prefs.Store(dst, old)
m.prefs.Delete(src)
m.appLock.Lock()
m.appPrefs = dst
m.appLock.Unlock()

binds := m.getBindings(dst)
if binds == nil {
Expand Down
4 changes: 4 additions & 0 deletions dialog/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,10 @@ func getFavoriteOrder() []string {
}

func hasAppFiles(a fyne.App) bool {
if a.UniqueID() == "testApp" {
dweymouth marked this conversation as resolved.
Show resolved Hide resolved
return false
}

return len(a.Storage().List()) > 0
}

Expand Down
1 change: 1 addition & 0 deletions dialog/file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,7 @@ func TestViewPreferences(t *testing.T) {
}

func TestFileFavorites(t *testing.T) {
_ = test.NewApp()
win := test.NewTempWindow(t, widget.NewLabel("Content"))

dlg := NewFileOpen(func(reader fyne.URIReadCloser, err error) {
Expand Down
4 changes: 2 additions & 2 deletions internal/animation/animation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ func TestGLDriver_StopAnimationImmediatelyAndInsideTick(t *testing.T) {
Tick: func(f float32) {},
}
run.Start(c)
go tick(run) // simulate a graphics draw loop
tick(run) // simulate a graphics draw loop

run.Stop(c)
go tick(run) // simulate a graphics draw loop
tick(run) // simulate a graphics draw loop

wg.Wait()
// animations stopped inside tick are really stopped in the next runner cycle
Expand Down
4 changes: 2 additions & 2 deletions internal/app/lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,9 @@ func (l *Lifecycle) QueueEvent(fn func()) {

// RunEventQueue runs the event queue. This should called inside a go routine.
// This function blocks.
func (l *Lifecycle) RunEventQueue() {
func (l *Lifecycle) RunEventQueue(run func(func())) {
for fn := range l.eventQueue.Out() {
fn()
run(fn)
}
}

Expand Down
4 changes: 3 additions & 1 deletion internal/app/lifecycle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ func TestLifecycle(t *testing.T) {

var entered, exited, start, stop, hookedStop, called bool
life.InitEventQueue()
go life.RunEventQueue()
go life.RunEventQueue(func(fn func()) {
fn()
})
life.QueueEvent(func() { called = true })
life.SetOnEnteredForeground(func() { entered = true })
life.OnEnteredForeground()()
Expand Down
4 changes: 0 additions & 4 deletions internal/async/pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@ func TestPool(t *testing.T) {
item := pool.Get()
assert.Equal(t, 0, item)

item = 5
pool.Put(item)
assert.Equal(t, item, pool.Get())

pool.New = func() int {
return -1
}
Expand Down
24 changes: 15 additions & 9 deletions internal/driver/glfw/canvas_other_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ import (
func TestGlCanvas_FocusHandlingWhenActivatingOrDeactivatingTheMenu(t *testing.T) {
w := createWindow("Test")
w.SetPadded(false)
w.SetMainMenu(
fyne.NewMainMenu(
fyne.NewMenu("test", fyne.NewMenuItem("item", func() {})),
fyne.NewMenu("other", fyne.NewMenuItem("item", func() {})),
),
)
c := w.Canvas().(*glCanvas)
runOnMain(func() {
w.SetMainMenu(
fyne.NewMainMenu(
fyne.NewMenu("test", fyne.NewMenuItem("item", func() {})),
fyne.NewMenu("other", fyne.NewMenuItem("item", func() {})),
),
)
})
c := w.Canvas()

ce1 := &focusable{id: "ce1"}
ce2 := &focusable{id: "ce2"}
Expand All @@ -35,7 +37,9 @@ func TestGlCanvas_FocusHandlingWhenActivatingOrDeactivatingTheMenu(t *testing.T)
assert.Equal(t, ce2, c.Focused())
assert.True(t, ce2.focused)

m.Items[0].(*menuBarItem).Tapped(&fyne.PointEvent{})
runOnMain(func() {
m.Items[0].(*menuBarItem).Tapped(&fyne.PointEvent{})
})
assert.True(t, m.IsActive())
ctxt := "activating the menu changes focus handler and focuses the menu bar item but does not remove focus from content"
assert.True(t, ce2.focused, ctxt)
Expand All @@ -46,7 +50,9 @@ func TestGlCanvas_FocusHandlingWhenActivatingOrDeactivatingTheMenu(t *testing.T)
assert.True(t, ce2.focused, ctxt)
assert.Equal(t, m.Items[1], c.Focused(), ctxt)

m.Items[0].(*menuBarItem).Tapped(&fyne.PointEvent{})
runOnMain(func() {
m.Items[0].(*menuBarItem).Tapped(&fyne.PointEvent{})
})
assert.False(t, m.IsActive())
ctxt = "deactivating the menu restores focus handler from content"
assert.True(t, ce2.focused, ctxt)
Expand Down
Loading
Loading