From 21cf60cf91d94cdf443ee340a607e9c1fd780bc5 Mon Sep 17 00:00:00 2001 From: nobe4 Date: Tue, 13 Aug 2024 12:52:32 +0200 Subject: [PATCH 1/5] test(integration): add error handling for responses --- internal/api/mock/calls.go | 31 +++++++++++++++++++++---------- internal/api/mock/mock.go | 4 ++-- test/integration/001/calls.json | 7 +++++++ 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/internal/api/mock/calls.go b/internal/api/mock/calls.go index 4f4d2d1..136f1b4 100644 --- a/internal/api/mock/calls.go +++ b/internal/api/mock/calls.go @@ -6,6 +6,8 @@ import ( "net/http" "os" "strings" + + "github.com/cli/go-gh/v2/pkg/api" ) type Call struct { @@ -20,7 +22,7 @@ type RawCall struct { Verb string `json:"verb"` Endpoint string `json:"endpoint"` Data any `json:"data"` - Error error `json:"error"` + Error RawError `json:"error"` Response RawResponse `json:"response"` } @@ -30,6 +32,10 @@ type RawResponse struct { Body any `json:"body"` } +type RawError struct { + StatusCode int `json:"status_code"` +} + func LoadCallsFromFile(path string) ([]Call, error) { rawCalls := []RawCall{} @@ -43,23 +49,28 @@ func LoadCallsFromFile(path string) ([]Call, error) { } calls := make([]Call, len(rawCalls)) - for i, call := range rawCalls { - body, err := json.Marshal(call.Response.Body) + for i, rawCall := range rawCalls { + body, err := json.Marshal(rawCall.Response.Body) if err != nil { return nil, err } - calls[i] = Call{ - Verb: call.Verb, - Endpoint: call.Endpoint, - Data: call.Data, - Error: call.Error, + call := Call{ + Verb: rawCall.Verb, + Endpoint: rawCall.Endpoint, + Data: rawCall.Data, Response: &http.Response{ - Header: http.Header(call.Response.Headers), - StatusCode: call.Response.StatusCode, + Header: http.Header(rawCall.Response.Headers), + StatusCode: rawCall.Response.StatusCode, Body: io.NopCloser(strings.NewReader(string(body))), }, } + + if rawCall.Error.StatusCode != 0 { + call.Error = &api.HTTPError{StatusCode: rawCall.Error.StatusCode} + } + + calls[i] = call } return calls, nil diff --git a/internal/api/mock/mock.go b/internal/api/mock/mock.go index 5fe3539..1cdc519 100644 --- a/internal/api/mock/mock.go +++ b/internal/api/mock/mock.go @@ -22,7 +22,7 @@ type MockError struct { } func (e *MockError) Error() string { - return fmt.Sprintf("mock error: %s %s: %s", e.verb, e.endpoint, e.message) + return fmt.Sprintf("mock error for call [%s %s]: %s", e.verb, e.endpoint, e.message) } func New(c []Call) api.Requestor { @@ -47,7 +47,7 @@ func (m *Mock) call(verb, endpoint string) (Call, error) { return Call{}, &MockError{ verb, endpoint, - fmt.Sprintf("unexpected call: mismatch, expected %s %s", call.Verb, call.Endpoint), + fmt.Sprintf("unexpected call: mismatch, expected [%s %s]", call.Verb, call.Endpoint), } } diff --git a/test/integration/001/calls.json b/test/integration/001/calls.json index 0d4060f..c381fc5 100644 --- a/test/integration/001/calls.json +++ b/test/integration/001/calls.json @@ -12,6 +12,13 @@ "body": [{ "id": "1", "subject": { "url": "enrichment#1" } }] } }, + { + "verb": "GET", + "endpoint": "https://api.github.com/notifications?all=true&page=2", + "error": { + "status_code": 404 + } + }, { "verb": "GET", "endpoint": "https://api.github.com/notifications?all=true&page=2", From e15d0357fe5cc0e7b72a359ae95dd72dcb52cc46 Mon Sep 17 00:00:00 2001 From: nobe4 Date: Sat, 14 Sep 2024 11:44:51 +0200 Subject: [PATCH 2/5] fix(sync): use the notification slice instead of the map This guarantees the order of execution, enabling the tests in a predictable way. It keeps the map to check if the notifications already exist in either lists, which might be refactored later. --- internal/notifications/sync.go | 39 +++++++++++++++++----------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/internal/notifications/sync.go b/internal/notifications/sync.go index b6e8a5f..932b7b1 100644 --- a/internal/notifications/sync.go +++ b/internal/notifications/sync.go @@ -24,55 +24,56 @@ state if the remote notification is newer than the local one. TODO: refactor this to `func (n Notifications) Sync(remote Notifications) {}` */ func Sync(local, remote Notifications) Notifications { + // TODO: do we need to have the whole map? remoteMap := remote.Map() localMap := local.Map() n := Notifications{} // Add any new notifications to the list - for remoteId, remote := range remoteMap { - if _, ok := localMap[remoteId]; !ok { + for i := range remote { + if _, ok := localMap[remote[i].Id]; !ok { // (1) Insert - slog.Debug("sync", "action", "insert", "id", remote.Id) + slog.Debug("sync", "action", "insert", "id", remote[i].Id) - remote.Meta.RemoteExists = true - n = append(n, remote) + remote[i].Meta.RemoteExists = true + n = append(n, remote[i]) } } - for localId, local := range localMap { - remote, remoteExist := remoteMap[localId] + for i := range local { + remote, remoteExist := remoteMap[local[i].Id] - local.Meta.RemoteExists = remoteExist + local[i].Meta.RemoteExists = remoteExist if remoteExist { // (3) Keep - if local.Meta.Hidden { - slog.Debug("sync", "action", "keeping hidden", "id", local.Id) - n = append(n, local) + if local[i].Meta.Hidden { + slog.Debug("sync", "action", "keeping hidden", "id", local[i].Id) + n = append(n, local[i]) continue } // (2) Update slog.Debug("sync", "action", "update", "id", remote.Id) - if local.Meta.Done && remote.UpdatedAt.After(local.UpdatedAt) { - slog.Debug("sync", "action", "reseting done", "id", local.Id) - local.Meta.Done = false + if local[i].Meta.Done && remote.UpdatedAt.After(local[i].UpdatedAt) { + slog.Debug("sync", "action", "reseting done", "id", local[i].Id) + local[i].Meta.Done = false } - remote.Meta = local.Meta + remote.Meta = local[i].Meta n = append(n, remote) } else { - if local.Meta.Done || local.Meta.Hidden { + if local[i].Meta.Done || local[i].Meta.Hidden { // (4) Drop - slog.Debug("sync", "action", "drop", "id", local.Id) + slog.Debug("sync", "action", "drop", "id", local[i].Id) continue } // (3) Keep - slog.Debug("sync", "action", "keep", "id", local.Id) - n = append(n, local) + slog.Debug("sync", "action", "keep", "id", local[i].Id) + n = append(n, local[i]) } } From 25da0f990fa00af4aa5045083bb8553eb2059e72 Mon Sep 17 00:00:00 2001 From: nobe4 Date: Sat, 14 Sep 2024 11:47:18 +0200 Subject: [PATCH 3/5] chore(log): add various logs --- internal/gh/enrichments.go | 4 +--- internal/notifications/notifications.go | 2 ++ test/integration/the_test.go | 15 +++++++++++++-- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/internal/gh/enrichments.go b/internal/gh/enrichments.go index 300f5b5..2b2a12d 100644 --- a/internal/gh/enrichments.go +++ b/internal/gh/enrichments.go @@ -20,7 +20,7 @@ func (c *Client) Enrich(n *notifications.Notification) error { return nil } - slog.Debug("enriching", "url", n.Subject.URL) + slog.Debug("enriching", "id", n.Id, "url", n.Subject.URL) resp, err := c.API.Request(http.MethodGet, n.Subject.URL, nil) if err != nil { return err @@ -38,8 +38,6 @@ func (c *Client) Enrich(n *notifications.Notification) error { return err } - slog.Debug("enriching", "id", n.Id) - n.Author = extra.User n.Subject.State = extra.State n.Subject.HtmlUrl = extra.HtmlUrl diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index 9881c4b..429f3ec 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -7,6 +7,7 @@ package notifications import ( "encoding/json" "fmt" + "log/slog" "slices" "strings" "time" @@ -82,6 +83,7 @@ func (n Notifications) Equal(others Notifications) bool { for i, n := range n { if !n.Equal(others[i]) { + slog.Info("notification not equal", "n", n.Debug(), "other", others[i].Debug()) return false } } diff --git a/test/integration/the_test.go b/test/integration/the_test.go index 4759988..3c623cd 100644 --- a/test/integration/the_test.go +++ b/test/integration/the_test.go @@ -3,6 +3,7 @@ package tests import ( "encoding/json" "fmt" + "log/slog" "os" "testing" @@ -23,6 +24,7 @@ type config struct { func setup(t *testing.T, conf config) (*manager.Manager, *apiMock.Mock, notifications.Notifications) { logger.Init(5) + slog.Info("---- Starting test ----", "test", t.Name()) configPath := fmt.Sprintf("./%s/config.yaml", conf.Id) callsPath := fmt.Sprintf("./%s/calls.json", conf.Id) @@ -46,13 +48,22 @@ func setup(t *testing.T, conf config) (*manager.Manager, *apiMock.Mock, notifica caller := &apiMock.Mock{Calls: calls} m.SetCaller(caller) - if err := m.Load(); err != nil { + if err = m.Load(); err != nil { t.Fatal(err) } - if err := m.Refresh(); err != nil { + + for _, n := range m.Notifications { + slog.Info("Loaded notification", "id", n.Id) + } + + if err = m.Refresh(); err != nil { t.Fatal(err) } + for _, n := range m.Notifications { + slog.Info("Refresh notification", "id", n.Id) + } + want := notifications.Notifications{} raw, err := os.ReadFile(wantPath) From 6b8d70ddae93fdcdcada9065d825e12bd56d3755 Mon Sep 17 00:00:00 2001 From: nobe4 Date: Sat, 14 Sep 2024 11:48:02 +0200 Subject: [PATCH 4/5] feat(test): add integration test with dropped notifications --- test/integration/002/cache.json | 74 ++++++++++++++++++++++++++++++++ test/integration/002/calls.json | 39 +++++++++++++++++ test/integration/002/config.yaml | 0 test/integration/002/want.json | 50 +++++++++++++++++++++ 4 files changed, 163 insertions(+) create mode 100644 test/integration/002/cache.json create mode 100644 test/integration/002/calls.json create mode 100644 test/integration/002/config.yaml create mode 100644 test/integration/002/want.json diff --git a/test/integration/002/cache.json b/test/integration/002/cache.json new file mode 100644 index 0000000..b96a2ff --- /dev/null +++ b/test/integration/002/cache.json @@ -0,0 +1,74 @@ +[ + { + "id": "1 exists", + "subject": { + "url": "enrichment#1", + "state": "open" + }, + "meta": { + "remote_exists": true, + "done": false, + "hidden": false + } + }, + { + "id": "1 missing", + "subject": { + "url": "enrichment#1", + "state": "open" + }, + "meta": { + "remote_exists": true, + "done": false, + "hidden": false + } + }, + { + "id": "2 exists", + "subject": { + "url": "enrichment#2", + "state": "open" + }, + "meta": { + "remote_exists": true, + "done": true, + "hidden": false + } + }, + { + "id": "2 missing", + "subject": { + "url": "enrichment#2", + "state": "open" + }, + "meta": { + "remote_exists": true, + "done": true, + "hidden": false + } + }, + { + "id": "3 exists", + "subject": { + "url": "enrichment#3", + "state": "open" + }, + "meta": { + "remote_exists": true, + "done": false, + "hidden": true + } + }, + { + "id": "3 missing", + "subject": { + "url": "enrichment#3", + "state": "open" + }, + "meta": { + "remote_exists": true, + "done": false, + "hidden": true + } + } +] diff --git a/test/integration/002/calls.json b/test/integration/002/calls.json new file mode 100644 index 0000000..cff699a --- /dev/null +++ b/test/integration/002/calls.json @@ -0,0 +1,39 @@ +[ + { + "verb": "GET", + "endpoint": "https://api.github.com/notifications?all=true", + "response": { + "status_code": 200, + "headers": {}, + "body": [ + { "id": "1 exists", "subject": { "url": "enrichment#1" } }, + { "id": "2 exists", "subject": { "url": "enrichment#2" } }, + { "id": "3 exists", "subject": { "url": "enrichment#3" } } + ] + } + }, + { + "verb": "GET", + "endpoint": "enrichment#1", + "response": { + "status_code": 200, + "body": { "state": "open" } + } + }, + { + "verb": "GET", + "endpoint": "enrichment#1", + "response": { + "status_code": 200, + "body": { "state": "open" } + } + }, + { + "verb": "GET", + "endpoint": "enrichment#3", + "response": { + "status_code": 200, + "body": { "state": "open" } + } + } +] diff --git a/test/integration/002/config.yaml b/test/integration/002/config.yaml new file mode 100644 index 0000000..e69de29 diff --git a/test/integration/002/want.json b/test/integration/002/want.json new file mode 100644 index 0000000..7b168be --- /dev/null +++ b/test/integration/002/want.json @@ -0,0 +1,50 @@ +[ + { + "id": "1 exists", + "subject": { + "url": "enrichment#1", + "state": "open" + }, + "meta": { + "remote_exists": true, + "done": false, + "hidden": false + } + }, + { + "id": "1 missing", + "subject": { + "url": "enrichment#1", + "state": "open" + }, + "meta": { + "remote_exists": false, + "done": false, + "hidden": false + } + }, + { + "id": "2 exists", + "subject": { + "url": "enrichment#2", + "state": "" + }, + "meta": { + "remote_exists": true, + "done": true, + "hidden": false + } + }, + { + "id": "3 exists", + "subject": { + "url": "enrichment#3", + "state": "open" + }, + "meta": { + "remote_exists": true, + "done": false, + "hidden": true + } + } +] From 9aade7c35c99805cc8da2f80d27b05a6b021920d Mon Sep 17 00:00:00 2001 From: nobe4 Date: Sat, 14 Sep 2024 11:49:02 +0200 Subject: [PATCH 5/5] fix: run linter --- internal/notifications/sync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/notifications/sync.go b/internal/notifications/sync.go index 932b7b1..0145343 100644 --- a/internal/notifications/sync.go +++ b/internal/notifications/sync.go @@ -24,7 +24,7 @@ state if the remote notification is newer than the local one. TODO: refactor this to `func (n Notifications) Sync(remote Notifications) {}` */ func Sync(local, remote Notifications) Notifications { - // TODO: do we need to have the whole map? + // TODO: do we need to have the whole map? remoteMap := remote.Map() localMap := local.Map()