From e349a188613d2889694a806219b88369b8863d8a Mon Sep 17 00:00:00 2001 From: Neale Pickett Date: Wed, 20 Oct 2021 11:29:55 -0600 Subject: [PATCH 1/6] Trying to isolate a race condition in tests --- .gitlab-ci.yml | 2 +- cmd/mothd/httpd_test.go | 50 ++++++++++++++++++++++++++++++++++++---- cmd/mothd/mothballs.go | 7 ++++++ cmd/mothd/server.go | 3 +++ cmd/mothd/server_test.go | 3 +++ cmd/mothd/theme.go | 4 ++++ cmd/mothd/transpiler.go | 4 ++++ 7 files changed, 68 insertions(+), 5 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b6d291d..7682c41 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -10,7 +10,7 @@ test: - main - merge_requests script: - - go test ./... + - go test -race ./... push: stage: push diff --git a/cmd/mothd/httpd_test.go b/cmd/mothd/httpd_test.go index 6c623c3..2337bec 100644 --- a/cmd/mothd/httpd_test.go +++ b/cmd/mothd/httpd_test.go @@ -4,10 +4,12 @@ import ( "bytes" "encoding/json" "fmt" + "io/ioutil" + "log" "net/http/httptest" "net/url" + "strings" "testing" - "time" "github.com/spf13/afero" ) @@ -33,7 +35,8 @@ func (hs *HTTPServer) TestRequest(path string, args map[string]string) *httptest } func TestHttpd(t *testing.T) { - hs := NewHTTPServer("/", NewTestServer()) + server := NewTestServer() + hs := NewHTTPServer("/", server) if r := hs.TestRequest("/", nil); r.Result().StatusCode != 200 { t.Error(r.Result()) @@ -106,11 +109,26 @@ func TestHttpd(t *testing.T) { if r := hs.TestRequest("/answer", map[string]string{"cat": "pategory", "points": "1", "answer": "answer123"}); r.Result().StatusCode != 200 { t.Error(r.Result()) + } else if strings.Contains(r.Body.String(), "incorrect answer") { + // Pernicious intermittent bug + t.Error("Incorrect answer that was actually correct") + for _, provider := range server.PuzzleProviders { + if mb, ok := provider.(*Mothballs); !ok { + t.Error("Provider is not a mothball") + } else { + cat, _ := mb.getCat("pategory") + f, _ := cat.Open("answers.txt") + defer f.Close() + answersBytes, _ := ioutil.ReadAll(f) + t.Errorf("Correct answers: %v", string(answersBytes)) + } + } + t.Error("Wrong answer") } else if r.Body.String() != `{"status":"success","data":{"short":"accepted","description":"1 points awarded in pategory"}}` { t.Error("Unexpected body", r.Body.String()) } - time.Sleep(TestMaintenanceInterval) + server.State.refresh() if r := hs.TestRequest("/content/pategory/2/puzzle.json", nil); r.Result().StatusCode != 200 { t.Error(r.Result()) @@ -122,13 +140,37 @@ func TestHttpd(t *testing.T) { } else if err := json.Unmarshal(r.Body.Bytes(), &state); err != nil { t.Error(err) } else if len(state.PointsLog) != 1 { - t.Error("Points log wrong length") + switch v := server.State.(type) { + case *State: + log.Print(v) + } + + t.Errorf("Points log wrong length. Wanted 1, got %v", state.PointsLog) } else if len(state.Puzzles["pategory"]) != 2 { t.Error("Didn't unlock next puzzle") } if r := hs.TestRequest("/answer", map[string]string{"cat": "pategory", "points": "1", "answer": "answer123"}); r.Result().StatusCode != 200 { t.Error(r.Result()) + } else if strings.Contains(r.Body.String(), "incorrect answer") { + // Pernicious intermittent bug + t.Error("Incorrect answer that was actually correct") + for _, provider := range server.PuzzleProviders { + if mb, ok := provider.(*Mothballs); !ok { + t.Error("Provider is not a mothball") + } else { + if cat, ok := mb.getCat("pategory"); !ok { + t.Error("opening pategory failed") + } else if f, err := cat.Open("answers.txt"); err != nil { + t.Error("opening answers.txt", err) + } else { + defer f.Close() + answersBytes, _ := ioutil.ReadAll(f) + t.Errorf("Correct answers: %#v len %d", string(answersBytes), len(answersBytes)) + } + } + } + t.Error("Wrong answer") } else if r.Body.String() != `{"status":"fail","data":{"short":"not accepted","description":"error awarding points: points already awarded to this team in this category"}}` { t.Error("Unexpected body", r.Body.String()) } diff --git a/cmd/mothd/mothballs.go b/cmd/mothd/mothballs.go index 67b7679..cd252fd 100644 --- a/cmd/mothd/mothballs.go +++ b/cmd/mothd/mothballs.go @@ -92,23 +92,30 @@ func (m *Mothballs) Inventory() []Category { func (m *Mothballs) CheckAnswer(cat string, points int, answer string) (bool, error) { zfs, ok := m.getCat(cat) if !ok { + log.Println("There's no such category") return false, fmt.Errorf("no such category: %s", cat) } + log.Println("Opening answers.txt") af, err := zfs.Open("answers.txt") if err != nil { + log.Println("I did not find an answer") return false, fmt.Errorf("no answers.txt file") } defer af.Close() + log.Println("I'm going to start looking for an answer") needle := fmt.Sprintf("%d %s", points, answer) scanner := bufio.NewScanner(af) for scanner.Scan() { + log.Println("testing equality between", scanner.Text(), needle) if scanner.Text() == needle { return true, nil } } + log.Println("I did not find the answer", answer) + return false, nil } diff --git a/cmd/mothd/server.go b/cmd/mothd/server.go index 608e6d6..49d3f54 100644 --- a/cmd/mothd/server.go +++ b/cmd/mothd/server.go @@ -68,6 +68,9 @@ type Maintainer interface { // It will only be called once, when execution begins. // It's okay to just exit if there's no maintenance to be done. Maintain(updateInterval time.Duration) + + // refresh is a shortcut used internally for testing + refresh() } // MothServer gathers together the providers that make up a MOTH server. diff --git a/cmd/mothd/server_test.go b/cmd/mothd/server_test.go index 2434e22..224d397 100644 --- a/cmd/mothd/server_test.go +++ b/cmd/mothd/server_test.go @@ -11,6 +11,9 @@ import ( const TestMaintenanceInterval = time.Millisecond * 1 const TestTeamID = "teamID" +// NewTestServer creates a new MothServer with NewTestMothballs and some initial state. +// +// See function definition for details. func NewTestServer() *MothServer { puzzles := NewTestMothballs() go puzzles.Maintain(TestMaintenanceInterval) diff --git a/cmd/mothd/theme.go b/cmd/mothd/theme.go index a70ca32..2b9b0ff 100644 --- a/cmd/mothd/theme.go +++ b/cmd/mothd/theme.go @@ -38,3 +38,7 @@ func (t *Theme) Open(name string) (ReadSeekCloser, time.Time, error) { func (t *Theme) Maintain(i time.Duration) { // No periodic tasks for a theme } + +func (t *Theme) refresh() { + // Nothing to do for a theme +} diff --git a/cmd/mothd/transpiler.go b/cmd/mothd/transpiler.go index 7103b20..ddeb3da 100644 --- a/cmd/mothd/transpiler.go +++ b/cmd/mothd/transpiler.go @@ -79,3 +79,7 @@ func (p TranspilerProvider) Mothball(cat string, w io.Writer) error { func (p TranspilerProvider) Maintain(updateInterval time.Duration) { // Nothing to do here. } + +func (p TranspilerProvider) refresh() { + // Nothing to do for a theme +} From 459d7747266e16f451835d29bca2e9df31121b90 Mon Sep 17 00:00:00 2001 From: Neale Pickett Date: Wed, 20 Oct 2021 11:30:53 -0600 Subject: [PATCH 2/6] Always run tests in CI, not just on main branch --- .gitlab-ci.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 7682c41..725caaa 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -5,10 +5,6 @@ stages: test: stage: test image: golang:1.17 - only: - refs: - - main - - merge_requests script: - go test -race ./... From 85f5b96a40be5b4f6bc114e805915c3ab658fba8 Mon Sep 17 00:00:00 2001 From: Neale Pickett Date: Tue, 10 May 2022 19:36:36 -0600 Subject: [PATCH 3/6] Stop running goroutines in unit tests --- cmd/mothd/httpd_test.go | 5 ++--- cmd/mothd/server_test.go | 13 ++++--------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/cmd/mothd/httpd_test.go b/cmd/mothd/httpd_test.go index f551df4..9ddee02 100644 --- a/cmd/mothd/httpd_test.go +++ b/cmd/mothd/httpd_test.go @@ -74,7 +74,7 @@ func TestHttpd(t *testing.T) { t.Error("Register failed", r.Body.String()) } - time.Sleep(TestMaintenanceInterval) + stateProvider.refresh() if r := hs.TestRequest("/state", nil); r.Result().StatusCode != 200 { t.Error(r.Result()) @@ -131,7 +131,6 @@ func TestHttpd(t *testing.T) { t.Error("Unexpected body", r.Body.String()) } - time.Sleep(TestMaintenanceInterval) stateProvider.refresh() if r := hs.TestRequest("/content/pategory/2/puzzle.json", nil); r.Result().StatusCode != 200 { @@ -149,7 +148,7 @@ func TestHttpd(t *testing.T) { log.Print(v) } - t.Errorf("Points log wrong length. Wanted 1, got %v", state.PointsLog) + t.Errorf("Points log wrong length. Wanted 1, got %v (length %d)", state.PointsLog, len(state.PointsLog)) } else if len(state.Puzzles["pategory"]) != 2 { t.Error("Didn't unlock next puzzle") } diff --git a/cmd/mothd/server_test.go b/cmd/mothd/server_test.go index 0c49285..92c8839 100644 --- a/cmd/mothd/server_test.go +++ b/cmd/mothd/server_test.go @@ -3,12 +3,10 @@ package main import ( "io/ioutil" "testing" - "time" "github.com/spf13/afero" ) -const TestMaintenanceInterval = time.Millisecond * 1 const TestTeamID = "teamID" // NewTestServer creates a new MothServer with NewTestMothballs and some initial state. @@ -17,17 +15,14 @@ const TestTeamID = "teamID" func NewTestServer() *MothServer { puzzles := NewTestMothballs() puzzles.refresh() - go puzzles.Maintain(TestMaintenanceInterval) state := NewTestState() afero.WriteFile(state, "teamids.txt", []byte("teamID\n"), 0644) afero.WriteFile(state, "messages.html", []byte("messages.html"), 0644) state.refresh() - go state.Maintain(TestMaintenanceInterval) theme := NewTestTheme() afero.WriteFile(theme.Fs, "/index.html", []byte("index.html"), 0644) - go theme.Maintain(TestMaintenanceInterval) return NewMothServer(Configuration{}, theme, state, puzzles) } @@ -54,6 +49,7 @@ func TestProdServer(t *testing.T) { teamID := TestTeamID server := NewTestServer() + state := server.State.(*State) handler := server.NewHandler(participantID, teamID) anonHandler := server.NewHandler("badParticipantId", "badTeamId") @@ -85,8 +81,7 @@ func TestProdServer(t *testing.T) { t.Error("index.html wrong contents", contents) } - // Wait for refresh to pick everything up - time.Sleep(TestMaintenanceInterval) + state.refresh() { es := handler.ExportState() @@ -139,7 +134,7 @@ func TestProdServer(t *testing.T) { t.Error("Right answer marked wrong", err) } - time.Sleep(TestMaintenanceInterval) + state.refresh() { es := handler.ExportState() @@ -168,7 +163,7 @@ func TestProdServer(t *testing.T) { t.Error("Right answer marked wrong:", err) } - time.Sleep(TestMaintenanceInterval) + state.refresh() { es := anonHandler.ExportState() From bde4b2c86ddc9da439bb3e13e897291b360a8283 Mon Sep 17 00:00:00 2001 From: Neale Pickett Date: Tue, 10 May 2022 19:48:51 -0600 Subject: [PATCH 4/6] A bit cleaner test interface, maybe --- cmd/mothd/httpd_test.go | 11 +++++------ cmd/mothd/server_test.go | 22 ++++++++++++++++------ 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/cmd/mothd/httpd_test.go b/cmd/mothd/httpd_test.go index 9ddee02..385e188 100644 --- a/cmd/mothd/httpd_test.go +++ b/cmd/mothd/httpd_test.go @@ -36,8 +36,7 @@ func (hs *HTTPServer) TestRequest(path string, args map[string]string) *httptest func TestHttpd(t *testing.T) { server := NewTestServer() - hs := NewHTTPServer("/", server) - stateProvider := server.State.(*State) + hs := NewHTTPServer("/", server.MothServer) if r := hs.TestRequest("/", nil); r.Result().StatusCode != 200 { t.Error(r.Result()) @@ -74,7 +73,7 @@ func TestHttpd(t *testing.T) { t.Error("Register failed", r.Body.String()) } - stateProvider.refresh() + server.refresh() if r := hs.TestRequest("/state", nil); r.Result().StatusCode != 200 { t.Error(r.Result()) @@ -131,7 +130,7 @@ func TestHttpd(t *testing.T) { t.Error("Unexpected body", r.Body.String()) } - stateProvider.refresh() + server.refresh() if r := hs.TestRequest("/content/pategory/2/puzzle.json", nil); r.Result().StatusCode != 200 { t.Error(r.Result()) @@ -183,7 +182,7 @@ func TestDevelMemHttpd(t *testing.T) { srv := NewTestServer() { - hs := NewHTTPServer("/", srv) + hs := NewHTTPServer("/", srv.MothServer) if r := hs.TestRequest("/mothballer/pategory.md", nil); r.Result().StatusCode != 404 { t.Error("Should have gotten a 404 for mothballer in prod mode") @@ -192,7 +191,7 @@ func TestDevelMemHttpd(t *testing.T) { { srv.Config.Devel = true - hs := NewHTTPServer("/", srv) + hs := NewHTTPServer("/", srv.MothServer) if r := hs.TestRequest("/mothballer/pategory.md", nil); r.Result().StatusCode != 500 { t.Log(r.Body.String()) diff --git a/cmd/mothd/server_test.go b/cmd/mothd/server_test.go index 92c8839..4be46c6 100644 --- a/cmd/mothd/server_test.go +++ b/cmd/mothd/server_test.go @@ -9,10 +9,14 @@ import ( const TestTeamID = "teamID" +type TestServer struct { + *MothServer +} + // NewTestServer creates a new MothServer with NewTestMothballs and some initial state. // // See function definition for details. -func NewTestServer() *MothServer { +func NewTestServer() TestServer { puzzles := NewTestMothballs() puzzles.refresh() @@ -24,7 +28,14 @@ func NewTestServer() *MothServer { theme := NewTestTheme() afero.WriteFile(theme.Fs, "/index.html", []byte("index.html"), 0644) - return NewMothServer(Configuration{}, theme, state, puzzles) + return TestServer{NewMothServer(Configuration{}, theme, state, puzzles)} +} + +func (ts TestServer) refresh() { + ts.State.(*State).refresh() + for _, pp := range ts.PuzzleProviders { + pp.(*Mothballs).refresh() + } } func TestDevelServer(t *testing.T) { @@ -49,7 +60,6 @@ func TestProdServer(t *testing.T) { teamID := TestTeamID server := NewTestServer() - state := server.State.(*State) handler := server.NewHandler(participantID, teamID) anonHandler := server.NewHandler("badParticipantId", "badTeamId") @@ -81,7 +91,7 @@ func TestProdServer(t *testing.T) { t.Error("index.html wrong contents", contents) } - state.refresh() + server.refresh() { es := handler.ExportState() @@ -134,7 +144,7 @@ func TestProdServer(t *testing.T) { t.Error("Right answer marked wrong", err) } - state.refresh() + server.refresh() { es := handler.ExportState() @@ -163,7 +173,7 @@ func TestProdServer(t *testing.T) { t.Error("Right answer marked wrong:", err) } - state.refresh() + server.refresh() { es := anonHandler.ExportState() From 243fdfd0063d1dbc8e7acebaf24c01c8b8ab230a Mon Sep 17 00:00:00 2001 From: Neale Pickett Date: Tue, 10 May 2022 19:57:07 -0600 Subject: [PATCH 5/6] Remove some debugging --- cmd/mothd/httpd_test.go | 36 ------------------------------------ cmd/mothd/mothballs.go | 7 ------- cmd/mothd/server_test.go | 1 + 3 files changed, 1 insertion(+), 43 deletions(-) diff --git a/cmd/mothd/httpd_test.go b/cmd/mothd/httpd_test.go index 385e188..e5a51a7 100644 --- a/cmd/mothd/httpd_test.go +++ b/cmd/mothd/httpd_test.go @@ -4,11 +4,9 @@ import ( "bytes" "encoding/json" "fmt" - "io/ioutil" "log" "net/http/httptest" "net/url" - "strings" "testing" "github.com/spf13/afero" @@ -111,21 +109,6 @@ func TestHttpd(t *testing.T) { if r := hs.TestRequest("/answer", map[string]string{"cat": "pategory", "points": "1", "answer": "answer123"}); r.Result().StatusCode != 200 { t.Error(r.Result()) - } else if strings.Contains(r.Body.String(), "incorrect answer") { - // Pernicious intermittent bug - t.Error("Incorrect answer that was actually correct") - for _, provider := range server.PuzzleProviders { - if mb, ok := provider.(*Mothballs); !ok { - t.Error("Provider is not a mothball") - } else { - cat, _ := mb.getCat("pategory") - f, _ := cat.Open("answers.txt") - defer f.Close() - answersBytes, _ := ioutil.ReadAll(f) - t.Errorf("Correct answers: %v", string(answersBytes)) - } - } - t.Error("Wrong answer") } else if r.Body.String() != `{"status":"success","data":{"short":"accepted","description":"1 points awarded in pategory"}}` { t.Error("Unexpected body", r.Body.String()) } @@ -154,25 +137,6 @@ func TestHttpd(t *testing.T) { if r := hs.TestRequest("/answer", map[string]string{"cat": "pategory", "points": "1", "answer": "answer123"}); r.Result().StatusCode != 200 { t.Error(r.Result()) - } else if strings.Contains(r.Body.String(), "incorrect answer") { - // Pernicious intermittent bug - t.Error("Incorrect answer that was actually correct") - for _, provider := range server.PuzzleProviders { - if mb, ok := provider.(*Mothballs); !ok { - t.Error("Provider is not a mothball") - } else { - if cat, ok := mb.getCat("pategory"); !ok { - t.Error("opening pategory failed") - } else if f, err := cat.Open("answers.txt"); err != nil { - t.Error("opening answers.txt", err) - } else { - defer f.Close() - answersBytes, _ := ioutil.ReadAll(f) - t.Errorf("Correct answers: %#v len %d", string(answersBytes), len(answersBytes)) - } - } - } - t.Error("Wrong answer") } else if r.Body.String() != `{"status":"fail","data":{"short":"not accepted","description":"error awarding points: points already awarded to this team in this category"}}` { t.Error("Unexpected body", r.Body.String()) } diff --git a/cmd/mothd/mothballs.go b/cmd/mothd/mothballs.go index bc1ff72..f204975 100644 --- a/cmd/mothd/mothballs.go +++ b/cmd/mothd/mothballs.go @@ -92,30 +92,23 @@ func (m *Mothballs) Inventory() []Category { func (m *Mothballs) CheckAnswer(cat string, points int, answer string) (bool, error) { zfs, ok := m.getCat(cat) if !ok { - log.Println("There's no such category") return false, fmt.Errorf("no such category: %s", cat) } - log.Println("Opening answers.txt") af, err := zfs.Open("answers.txt") if err != nil { - log.Println("I did not find an answer") return false, fmt.Errorf("no answers.txt file") } defer af.Close() - log.Println("I'm going to start looking for an answer") needle := fmt.Sprintf("%d %s", points, answer) scanner := bufio.NewScanner(af) for scanner.Scan() { - log.Println("testing equality between", scanner.Text(), needle) if scanner.Text() == needle { return true, nil } } - log.Println("I did not find the answer", answer) - return false, nil } diff --git a/cmd/mothd/server_test.go b/cmd/mothd/server_test.go index 4be46c6..caaf61a 100644 --- a/cmd/mothd/server_test.go +++ b/cmd/mothd/server_test.go @@ -36,6 +36,7 @@ func (ts TestServer) refresh() { for _, pp := range ts.PuzzleProviders { pp.(*Mothballs).refresh() } + ts.Theme.(*Theme).refresh() } func TestDevelServer(t *testing.T) { From cbe231ef12b11f6887a24ef0d93740b11f42d162 Mon Sep 17 00:00:00 2001 From: Neale Pickett Date: Tue, 10 May 2022 20:05:16 -0600 Subject: [PATCH 6/6] Remove more debugging --- cmd/mothd/httpd_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/cmd/mothd/httpd_test.go b/cmd/mothd/httpd_test.go index e5a51a7..b56f56c 100644 --- a/cmd/mothd/httpd_test.go +++ b/cmd/mothd/httpd_test.go @@ -4,7 +4,6 @@ import ( "bytes" "encoding/json" "fmt" - "log" "net/http/httptest" "net/url" "testing" @@ -125,11 +124,6 @@ func TestHttpd(t *testing.T) { } else if err := json.Unmarshal(r.Body.Bytes(), &state); err != nil { t.Error(err) } else if len(state.PointsLog) != 1 { - switch v := server.State.(type) { - case *State: - log.Print(v) - } - t.Errorf("Points log wrong length. Wanted 1, got %v (length %d)", state.PointsLog, len(state.PointsLog)) } else if len(state.Puzzles["pategory"]) != 2 { t.Error("Didn't unlock next puzzle")