ai/live: Use synctest in discovery #3739
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
dependencies
j0sh
force-pushed
the
ja/discovery-synctest
branch
from
a1ec486 to
d8f25d8
Compare
|
Rebased onto latest master to fix merge conflicts, no other changes |
mjh1
reviewed
Sep 18, 2025
discovery/discovery_test.go
Outdated
| assert.Nil(err, "Should not be error") | ||
| assert.Len(infos, 1, "Should return one orchestrator") | ||
| assert.Equal("transcoderfromtestserver", infos[0].RemoteInfo.Transcoder) | ||
| wgWait(&wg) |
There was a problem hiding this comment.
do we need to check the returned bool?
There was a problem hiding this comment.
good idea, checked in a6110ec
mjh1
reviewed
Sep 18, 2025
| first := true | ||
| serverGetOrchInfo = func(ctx context.Context, bcast common.Broadcaster, orchestratorServer *url.URL, params server.GetOrchestratorInfoParams) (*net.OrchestratorInfo, error) { | ||
| mu.Lock() | ||
| if first { |
There was a problem hiding this comment.
why were we pausing on the first request, don't really understand that
There was a problem hiding this comment.
me neither - best guess is some early test needed it or had a race condition (probably the deadlock one), and it got copied everywhere else.
I double checked the rest of these tests and seems that I mistakenly removed the sleep from the deadlock test; that apparently was meant to drop one of the orchs so it wouldn't pass discovery. Re-added it, hopefully with a little better explanation in a6110ec (I still have no idea how this could have caused a deadlock though.)
victorges
approved these changes
Sep 19, 2025
The golang 1.25 update turned out into something of an ordeal, so splitting it up from #3739 to allow for better review. While everything compiled in 1.25, some tests and CI steps broke. * Go lint needed to be updated to v2+ to support golang 1.25 * Bumping go vet from v1 to v2 entailed a bunch of CLI changes. Tried to make those 1-to-1 as much as possible, however: Go fmt now needs to be invoked separately for golint. We check go fmt in the CI step right before lint ... so just remove the go fmt step? The revive linter flagged a ton of missing comments. But it seems the linter only applies to the `pm` and `verification` packages which hardly change. Rather than update those packages, remove the use of the `revive` linter. * Some linting step (not sure which one? vet?) now complains about format string functions being used incorrectly, eg using a variable in place of a format string literal. Those were fixed in the code. * The `rand.Seed` function apparently became a no-op as of golang 1.24 and that broke most of our tests that use the RNG. Removed the init() function that sets the seed, and added a package level RNG context (using the same seed as the old init) and updated any failing tests to use that context. Not all uses of the RNG were updated, just ones that broke tests. The global RNG is safely initialized so we can continue to use it by default in the code that still uses it without test coverage.
j0sh
force-pushed
the
ja/discovery-synctest
branch
from
f3310ea to
424ddbc
Compare
j0sh
changed the title
The new synctest package, introduced in golang 1.25, makes tests much faster and more reliable. All the discovery tests that took more than 100ms on my machine were converted to use synctest. This surfaced a number of race conditions in both the tests and the code itself so these were also fixed. Timings with `go test -timeout 10s -count 1 -race` Before: ok github.com/livepeer/go-livepeer/discovery 7.087s After: ok github.com/livepeer/go-livepeer/discovery 0.817s Since syncest depends on a nested call similar to a sub-test, I opted to just rename the top level test function in the interests of not having to re-indent the entire file, or having yet another level of indentation around the file.
j0sh
force-pushed
the
ja/discovery-synctest
branch
from
424ddbc to
b6015f2
Compare
|
Since the update for golang 1.25 became more involved than I was expecting, separated that out in its own PR in #3745 so this one has just the synctest changes. Once that goes in, this one will go in too. |
j0sh
added a commit
that referenced
this pull request
Sep 25, 2025
The golang 1.25 update turned out into something of an ordeal, so splitting it up from #3739 to allow for better review. While everything compiled in 1.25, some tests and CI steps broke. * Go lint needed to be updated to v2+ to support golang 1.25 * Bumping go vet from v1 to v2 entailed a bunch of CLI changes. Tried to make those 1-to-1 as much as possible, however: Go fmt now needs to be invoked separately for golint. We check go fmt in the CI step right before lint ... so just remove the go fmt step? The revive linter flagged a ton of missing comments. But it seems the linter only applies to the `pm` and `verification` packages which hardly change. Rather than update those packages, remove the use of the `revive` linter. * Some linting step (not sure which one? vet?) now complains about format string functions being used incorrectly, eg using a variable in place of a format string literal. Those were fixed in the code. * The `rand.Seed` function apparently became a no-op as of golang 1.24 and that broke most of our tests that use the RNG. Removed the init() function that sets the seed, and added a package level RNG context (using the same seed as the old init) and updated any failing tests to use that context. Not all uses of the RNG were updated, just ones that broke tests. The global RNG is safely initialized so we can continue to use it by default in the code that still uses it without test coverage. * Fix race condition in discovery uncovered by golang 1.25
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3739 +/- ##
===================================================
- Coverage 31.14906% 31.14775% -0.00131%
===================================================
Files 158 158
Lines 47552 47554 +2
===================================================
Hits 14812 14812
- Misses 31852 31854 +2
Partials 888 888
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The new synctest package, introduced in golang 1.25, makes tests involving time and concurrency much faster and more reliable. All the discovery tests that took more than 100ms on my machine were converted to use synctest.
This surfaced a number of race conditions in both the tests and the code itself so these were also fixed.
Timings with
go test -timeout 10s -count 1 -raceBefore:
ok github.com/livepeer/go-livepeer/discovery 7.087s
After:
ok github.com/livepeer/go-livepeer/discovery 0.817s
Since syncest depends on a nested call similar to a sub-test, I opted to just rename the top level test function in the interests of not having to re-indent the entire file, or having yet another level of indentation around the file.