`check` failed on main with two failures in one package, and they are one
cause wearing two shapes:
service_test.go:66: state = "satisfied", want wanted
testing.go:1369: TempDir RemoveAll cleanup: ... directory not empty
Every test in service_test.go is about the durable Request that
StartDownload leaves behind, and none is about the download. But the
fixture is an anchored four-track request with a healthy provider, which
is precisely what AutoPickable says yes to -- so Manager.Start fired
`go m.grab(...)`, detached and with context.WithoutCancel, and the tests
raced it. Measured: the request reaches "satisfied" about 100ms after
StartDownload returns, so the first failure is the assertion reading the
next state, and the second is that same goroutine still writing into
t.TempDir() after the test returned.
The fixture now puts the candidate outside the auto-pick size window, so
the grab never starts. That is better than waiting for it: with no
goroutine there is nothing to be slow, and the tests state what they mean
without a timing assumption underneath. A test that does want the
download uses managerFixture and sets its own preferences.
It passed 20 runs under CPU load, but so did the broken version -- this
is a CI-only failure locally, so the cause was proved directly instead:
with the fixture's old preferences the request is observably "satisfied"
within 100ms of StartDownload, which is what CI read.
213 lines
6.0 KiB
Go
213 lines
6.0 KiB
Go
package download
|
|
|
|
import (
|
|
"context"
|
|
"testing"
|
|
)
|
|
|
|
// newServiceFixture wires a Service over the same manager/store a
|
|
// managerFixture uses, so a manual download can be started and watched
|
|
// through to completion with no network anywhere.
|
|
type serviceFixture struct {
|
|
managerFixture
|
|
|
|
svc *Service
|
|
}
|
|
|
|
func newServiceFixture(t *testing.T) serviceFixture {
|
|
t.Helper()
|
|
|
|
mf := newManagerFixture(t)
|
|
svc := NewService(slogDiscard(), mf.manager, mf.store, NewMemSecretStore())
|
|
|
|
// Every test here is about the durable Request that `StartDownload`
|
|
// leaves behind, and none of them is about the download itself -- but
|
|
// their fixture is an anchored four-track request with a healthy
|
|
// provider, which is exactly what `AutoPickable` says yes to. So
|
|
// `Manager.Start` was firing `go m.grab(...)`, detached and with
|
|
// `context.WithoutCancel`, and the test then raced it.
|
|
//
|
|
// It lost, twice, in CI (`check` on c03c0b8, and nowhere locally):
|
|
//
|
|
// service_test.go:66: state = "satisfied", want wanted
|
|
// testing.go:1369: TempDir RemoveAll cleanup: ... directory not empty
|
|
//
|
|
// The first is the request reaching its *next* state before the
|
|
// assertion read it; the second is that same goroutine still writing
|
|
// into `t.TempDir()` after the test returned. One cause, two shapes.
|
|
//
|
|
// Putting the candidate outside the auto-pick size window stops the
|
|
// grab from ever starting, which is better than waiting for it: there
|
|
// is no goroutine to be slow, so the tests state what they mean
|
|
// ("the request exists, in this state") without a timing assumption
|
|
// underneath. A test that does want the download has `managerFixture`
|
|
// and sets its own preferences.
|
|
mf.manager.SetPreferences(AutoDownloadPrefs{MaxSizeMB: 1})
|
|
|
|
return serviceFixture{managerFixture: mf, svc: svc}
|
|
}
|
|
|
|
// A manual download for something anchored by MBID must leave a
|
|
// durable Request behind, whether or not the download itself succeeds
|
|
// — that is the whole point of ensureRequest: a manual attempt that
|
|
// finds nothing right now is not just lost, the reconciler picks it up
|
|
// later on its normal schedule.
|
|
func TestStartDownloadCreatesRequestForAnchoredDownload(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
f := newServiceFixture(t)
|
|
ctx := context.Background()
|
|
|
|
provider := fakeWithAlbum(1, "source", ".flac")
|
|
f.manager.installProvider(Config{ID: 1, Priority: 50}, provider)
|
|
|
|
dl := fourTrackDownload()
|
|
|
|
if _, err := f.svc.StartDownload(SearchRequest{
|
|
LibraryID: 1,
|
|
ReleaseGroupMBID: "rg-1",
|
|
Artist: dl.Artist,
|
|
Album: dl.Album,
|
|
Expected: dl.Expected,
|
|
}); err != nil {
|
|
t.Fatalf("StartDownload: %v", err)
|
|
}
|
|
|
|
req, found, err := f.store.FindRequest(ctx, "rg-1", 1)
|
|
if err != nil {
|
|
t.Fatalf("FindRequest: %v", err)
|
|
}
|
|
|
|
if !found {
|
|
t.Fatal("manual anchored download did not create a durable request")
|
|
}
|
|
|
|
if req.Entity != EntityReleaseGroup {
|
|
t.Errorf("entity = %q, want release-group", req.Entity)
|
|
}
|
|
|
|
if req.State != RequestStateWanted {
|
|
t.Errorf("state = %q, want wanted", req.State)
|
|
}
|
|
}
|
|
|
|
// A free-text download (no MBID) has nothing stable to attach a
|
|
// request to, and must not create one.
|
|
func TestStartDownloadFreeTextCreatesNoRequest(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
f := newServiceFixture(t)
|
|
ctx := context.Background()
|
|
|
|
provider := fakeWithAlbum(1, "source", ".flac")
|
|
f.manager.installProvider(Config{ID: 1, Priority: 50}, provider)
|
|
|
|
if _, err := f.svc.StartDownload(SearchRequest{
|
|
LibraryID: 1,
|
|
Query: "some free text search",
|
|
}); err != nil {
|
|
t.Fatalf("StartDownload: %v", err)
|
|
}
|
|
|
|
all, err := f.store.ListRequests(ctx)
|
|
if err != nil {
|
|
t.Fatalf("ListRequests: %v", err)
|
|
}
|
|
|
|
if len(all) != 0 {
|
|
t.Errorf("free-text download created %d requests, want 0", len(all))
|
|
}
|
|
}
|
|
|
|
// A manual download must not un-pause a request the user deliberately
|
|
// paused: it runs its one interactive attempt regardless, but the
|
|
// request's own state is left alone.
|
|
func TestStartDownloadDoesNotUnpauseExistingRequest(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
f := newServiceFixture(t)
|
|
ctx := context.Background()
|
|
|
|
id, err := f.store.AddRequest(ctx, Request{
|
|
MBID: "rg-1",
|
|
Entity: EntityReleaseGroup,
|
|
LibraryID: 1,
|
|
Artist: "Radiohead",
|
|
Title: "OK Computer",
|
|
})
|
|
if err != nil {
|
|
t.Fatalf("AddRequest: %v", err)
|
|
}
|
|
|
|
if err := f.store.SetRequestState(
|
|
ctx, id, RequestStatePaused, "",
|
|
); err != nil {
|
|
t.Fatalf("SetRequestState: %v", err)
|
|
}
|
|
|
|
provider := fakeWithAlbum(1, "source", ".flac")
|
|
f.manager.installProvider(Config{ID: 1, Priority: 50}, provider)
|
|
|
|
dl := fourTrackDownload()
|
|
|
|
if _, err := f.svc.StartDownload(SearchRequest{
|
|
LibraryID: 1,
|
|
ReleaseGroupMBID: "rg-1",
|
|
Artist: dl.Artist,
|
|
Album: dl.Album,
|
|
Expected: dl.Expected,
|
|
}); err != nil {
|
|
t.Fatalf("StartDownload: %v", err)
|
|
}
|
|
|
|
req, err := f.store.GetRequest(ctx, id)
|
|
if err != nil {
|
|
t.Fatalf("GetRequest: %v", err)
|
|
}
|
|
|
|
if req.State != RequestStatePaused {
|
|
t.Errorf(
|
|
"a manual download un-paused the request: state = %q, want paused",
|
|
req.State,
|
|
)
|
|
}
|
|
}
|
|
|
|
// A manual download that clearly wins auto-pick still satisfies the
|
|
// durable request it was attached to when it completes — the same
|
|
// SatisfyRequest call the reconciler relies on.
|
|
func TestManualDownloadSatisfiesRequestOnSuccess(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
f := newServiceFixture(t)
|
|
ctx := context.Background()
|
|
|
|
provider := fakeWithAlbum(1, "source", ".flac")
|
|
f.manager.installProvider(Config{ID: 1, Priority: 50}, provider)
|
|
|
|
dl := fourTrackDownload()
|
|
|
|
result, err := f.svc.StartDownload(SearchRequest{
|
|
LibraryID: 1,
|
|
ReleaseGroupMBID: "rg-1",
|
|
Artist: dl.Artist,
|
|
Album: dl.Album,
|
|
Expected: dl.Expected,
|
|
})
|
|
if err != nil {
|
|
t.Fatalf("StartDownload: %v", err)
|
|
}
|
|
|
|
if !result.AutoPicked {
|
|
t.Fatal("expected a clear single-provider winner to auto-pick")
|
|
}
|
|
|
|
waitForDownloadState(t, f.store, result.DownloadID, StateComplete)
|
|
|
|
waitFor(t, func() bool {
|
|
req, found, err := f.store.FindRequest(ctx, "rg-1", 1)
|
|
|
|
return err == nil && found && req.State == RequestStateSatisfied
|
|
}, "request was never satisfied after its manual download completed")
|
|
}
|