From 1ca4861074ce1c1e1edcbb3e70e16459936ed5da Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sun, 13 Mar 2022 15:44:11 +0100 Subject: [PATCH] Improve Twitch.tv clip tests --- CHANGELOG.md | 1 + internal/resolvers/twitch/clip_loader.go | 10 +- internal/resolvers/twitch/clip_resolver.go | 3 +- .../resolvers/twitch/clip_resolver_test.go | 210 ++++++++++++++---- internal/resolvers/twitch/data_test.go | 40 ++++ internal/resolvers/twitch/initialize.go | 2 +- internal/resolvers/twitch/initialize_test.go | 39 ++++ .../resolvers/twitch/parse_clip_slug_test.go | 47 +++- 8 files changed, 280 insertions(+), 72 deletions(-) create mode 100644 internal/resolvers/twitch/data_test.go create mode 100644 internal/resolvers/twitch/initialize_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f90f57d6..2bb00d80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Fix: SevenTV emotes now resolve correctly. (#281) - Dev: Improve BetterTTV emote tests. (#282) - Minor: BetterTTV cache key changed from plural to singular form. (#282) +- Dev: Improve Twitch.tv clip tests. (#283) ## 1.2.3 diff --git a/internal/resolvers/twitch/clip_loader.go b/internal/resolvers/twitch/clip_loader.go index 81e46bc3..2a07dff2 100644 --- a/internal/resolvers/twitch/clip_loader.go +++ b/internal/resolvers/twitch/clip_loader.go @@ -41,10 +41,7 @@ func (l *ClipLoader) Load(ctx context.Context, clipSlug string, r *http.Request) "error", err, ) - return &resolver.Response{ - Status: http.StatusInternalServerError, - Message: "An internal error occured while fetching the Twitch clip", - }, cache.NoSpecialDur, nil + return resolver.Errorf("Twitch clip load error: %s", err) } if len(response.Data.Clips) != 1 { @@ -64,10 +61,7 @@ func (l *ClipLoader) Load(ctx context.Context, clipSlug string, r *http.Request) var tooltip bytes.Buffer if err := twitchClipsTooltip.Execute(&tooltip, data); err != nil { - return &resolver.Response{ - Status: http.StatusInternalServerError, - Message: "twitch clip template error " + resolver.CleanResponse(err.Error()), - }, cache.NoSpecialDur, nil + return resolver.Errorf("Twitch clip template error: %s", err) } return &resolver.Response{ diff --git a/internal/resolvers/twitch/clip_resolver.go b/internal/resolvers/twitch/clip_resolver.go index 8ee2d312..2a3abfdb 100644 --- a/internal/resolvers/twitch/clip_resolver.go +++ b/internal/resolvers/twitch/clip_resolver.go @@ -11,7 +11,6 @@ import ( "github.com/Chatterino/api/pkg/cache" "github.com/Chatterino/api/pkg/config" "github.com/Chatterino/api/pkg/resolver" - "github.com/nicklaw5/helix" ) var ( @@ -71,7 +70,7 @@ func (r *ClipResolver) Name() string { return "twitch:clip" } -func NewClipResolver(ctx context.Context, cfg config.APIConfig, pool db.Pool, helixAPI *helix.Client) *ClipResolver { +func NewClipResolver(ctx context.Context, cfg config.APIConfig, pool db.Pool, helixAPI TwitchAPIClient) *ClipResolver { clipLoader := &ClipLoader{ helixAPI: helixAPI, } diff --git a/internal/resolvers/twitch/clip_resolver_test.go b/internal/resolvers/twitch/clip_resolver_test.go index f3908e61..ecfdcf39 100644 --- a/internal/resolvers/twitch/clip_resolver_test.go +++ b/internal/resolvers/twitch/clip_resolver_test.go @@ -2,67 +2,179 @@ package twitch import ( "context" + "errors" + "net/http" "net/url" "testing" "github.com/Chatterino/api/internal/logger" + "github.com/Chatterino/api/internal/mocks" + "github.com/Chatterino/api/pkg/config" + "github.com/Chatterino/api/pkg/utils" qt "github.com/frankban/quicktest" + "github.com/golang/mock/gomock" + "github.com/jackc/pgx/v4" + "github.com/nicklaw5/helix" + "github.com/pashagolub/pgxmock" ) -const goodSlugV1 = "GoodSlugV1" -const goodSlugV2 = "GoodSlugV2-HVUvT7bYQnMn6nwp" - -var validClips = []string{ - "https://clips.twitch.tv/" + goodSlugV1, - "https://clips.twitch.tv/" + goodSlugV2, - "https://twitch.tv/pajlada/clip/" + goodSlugV1, - "https://twitch.tv/pajlada/clip/" + goodSlugV2, - "https://twitch.tv/zneix/clip/" + goodSlugV1, - "https://twitch.tv/zneix/clip/" + goodSlugV2, - "https://m.twitch.tv/pajlada/clip/" + goodSlugV1, - "https://m.twitch.tv/pajlada/clip/" + goodSlugV2, - "https://m.twitch.tv/zneix/clip/" + goodSlugV1, - "https://m.twitch.tv/zneix/clip/" + goodSlugV2, - "https://m.twitch.tv/clip/" + goodSlugV1, - "https://m.twitch.tv/clip/" + goodSlugV2, - "https://m.twitch.tv/clip/clip/" + goodSlugV1, - "https://m.twitch.tv/clip/clip/" + goodSlugV2, -} +func TestClipResolver(t *testing.T) { + ctx := logger.OnContext(context.Background(), logger.NewTest()) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + c := qt.New(t) -var invalidClips = []string{ - "https://clips.twitch.tv/pajlada/clip/VastBitterVultureMau5", - "https://clips.twitch.tv/", - "https://twitch.tv/nam____________________________________________/clip/someSlugNam", - "https://twitch.tv/supinic/clip/", - "https://twitch.tv/pajlada/clips/VastBitterVultureMau5", - "https://twitch.tv/zneix/clip/ImpossibleOilyAlpacaTF2John-jIlgtnSAQ52BThHhifyouseethisvivon", - "https://twitch.tv/clip/slug", - "https://gql.twitch.tv/VastBitterVultureMau5", - "https://gql.twitch.tv/ThreeLetterAPI/clip/VastBitterVultureMau5", - "https://m.twitch.tv/VastBitterVultureMau5", - "https://m.twitch.tv/username/clip/clip/slug", - "https://m.twitch.tv/username/notclip/slug", -} + pool, _ := pgxmock.NewPool() + cfg := config.APIConfig{} + helixClient := mocks.NewMockTwitchAPIClient(ctrl) -func testCheck(ctx context.Context, resolver *ClipResolver, c *qt.C, urlString string) bool { - u, err := url.Parse(urlString) - c.Assert(u, qt.IsNotNil) - c.Assert(err, qt.IsNil) + resolver := NewClipResolver(ctx, cfg, pool, helixClient) - return resolver.Check(ctx, u) -} + c.Assert(resolver, qt.IsNotNil) -func TestCheck(t *testing.T) { - ctx := logger.OnContext(context.Background(), logger.NewTest()) - c := qt.New(t) + c.Run("Name", func(c *qt.C) { + c.Assert(resolver.Name(), qt.Equals, "twitch:clip") + }) + + c.Run("Check", func(c *qt.C) { + type checkTest struct { + label string + input *url.URL + expected bool + } + + tests := []checkTest{} + + for _, b := range validClipBase { + tests = append(tests, checkTest{ + label: "valid", + input: utils.MustParseURL(b + goodSlugV1), + expected: true, + }) + tests = append(tests, checkTest{ + label: "valid", + input: utils.MustParseURL(b + goodSlugV2), + expected: true, + }) + } + + for _, b := range invalidClips { + tests = append(tests, checkTest{ + label: "invalid", + input: utils.MustParseURL(b), + expected: false, + }) + } + + for _, test := range tests { + c.Run(test.label, func(c *qt.C) { + output := resolver.Check(ctx, test.input) + c.Assert(output, qt.Equals, test.expected) + }) + } + }) + + c.Run("Run", func(c *qt.C) { + c.Run("Error", func(c *qt.C) { + type runTest struct { + label string + inputURL *url.URL + inputEmoteHash string + inputReq *http.Request + expectedError error + } + + tests := []runTest{ + { + label: "Non-matching link", + inputURL: utils.MustParseURL("https://clips.twitch.tv/user/566ca04265dbbdab32ec054a"), + expectedError: errInvalidTwitchClip, + }, + } + + const q = `SELECT value FROM cache WHERE key=$1` + + for _, test := range tests { + c.Run(test.label, func(c *qt.C) { + outputBytes, outputError := resolver.Run(ctx, test.inputURL, test.inputReq) + c.Assert(outputError, qt.Equals, test.expectedError) + c.Assert(outputBytes, qt.IsNil) + }) + } + }) + + c.Run("Not cached", func(c *qt.C) { + type runTest struct { + label string + inputURL *url.URL + inputSlug string + inputReq *http.Request + expectedClipResponse *helix.ClipsResponse + expectedClipError error + expectedBytes []byte + expectedError error + rowsReturned int + } - resolver := &ClipResolver{} + tests := []runTest{ + { + label: "Emote", + inputURL: utils.MustParseURL("https://clips.twitch.tv/GoodSlugV1"), + inputSlug: "GoodSlugV1", + inputReq: nil, + expectedClipResponse: &helix.ClipsResponse{ + Data: helix.ManyClips{ + Clips: []helix.Clip{ + { + Title: "Title", + CreatorName: "CreatorName", + BroadcasterName: "BroadcasterName", + Duration: 5, + CreatedAt: "202", // will fail + ViewCount: 420, + ThumbnailURL: "https://example.com/thumbnail.png", + }, + }, + }, + }, + expectedClipError: nil, + expectedBytes: []byte(`{"status":200,"thumbnail":"https://example.com/thumbnail.png","tooltip":"%3Cdiv%20style=%22text-align:%20left%3B%22%3E%3Cb%3ETitle%3C%2Fb%3E%3Chr%3E%3Cb%3EClipped%20by:%3C%2Fb%3E%20CreatorName%3Cbr%3E%3Cb%3EChannel:%3C%2Fb%3E%20BroadcasterName%3Cbr%3E%3Cb%3EDuration:%3C%2Fb%3E%205s%3Cbr%3E%3Cb%3ECreated:%3C%2Fb%3E%20%3Cbr%3E%3Cb%3EViews:%3C%2Fb%3E%20420%3C%2Fdiv%3E"}`), + expectedError: nil, + }, + { + label: "GetClipsError", + inputURL: utils.MustParseURL("https://clips.twitch.tv/GoodSlugV1GetClipsError"), + inputSlug: "GoodSlugV1GetClipsError", + inputReq: nil, + expectedClipResponse: nil, + expectedClipError: errors.New("error"), + expectedBytes: []byte(`{"status":500,"message":"Twitch clip load error: error"}`), + expectedError: nil, + }, + // { + // label: "Bad JSON", + // inputURL: utils.MustParseURL("https://betterttv.com/emotes/bad"), + // inputSlug: "bad", + // inputReq: nil, + // expectedBytes: []byte(`{"status":500,"message":"betterttv api unmarshal error: invalid character \u0026#39;x\u0026#39; looking for beginning of value"}`), + // expectedError: nil, + // }, + } - for _, u := range validClips { - c.Assert(testCheck(ctx, resolver, c, u), qt.IsTrue, qt.Commentf("%v must be seen as a clip", u)) - } + const q = `SELECT value FROM cache WHERE key=$1` - for _, u := range invalidClips { - c.Assert(testCheck(ctx, resolver, c, u), qt.IsFalse, qt.Commentf("%v must not be seen as a clip", u)) - } + for _, test := range tests { + c.Run(test.label, func(c *qt.C) { + helixClient.EXPECT().GetClips(&helix.ClipsParams{IDs: []string{test.inputSlug}}).Times(1).Return(test.expectedClipResponse, test.expectedClipError) + pool.ExpectQuery("SELECT").WillReturnError(pgx.ErrNoRows) + pool.ExpectExec("INSERT INTO cache"). + WithArgs("twitch:clip:"+test.inputSlug, test.expectedBytes, pgxmock.AnyArg()). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + outputBytes, outputError := resolver.Run(ctx, test.inputURL, test.inputReq) + c.Assert(outputError, qt.Equals, test.expectedError) + c.Assert(outputBytes, qt.DeepEquals, test.expectedBytes) + }) + } + }) + }) } diff --git a/internal/resolvers/twitch/data_test.go b/internal/resolvers/twitch/data_test.go new file mode 100644 index 00000000..e040e185 --- /dev/null +++ b/internal/resolvers/twitch/data_test.go @@ -0,0 +1,40 @@ +package twitch + +const goodSlugV1 = "GoodSlugV1" +const goodSlugV2 = "GoodSlugV2-HVUvT7bYQnMn6nwp" + +var validClipBase = []string{ + "https://clips.twitch.tv/", + "https://twitch.tv/pajlada/clip/", + "https://twitch.tv/zneix/clip/", + "https://m.twitch.tv/pajlada/clip/", + "https://m.twitch.tv/zneix/clip/", + "https://m.twitch.tv/clip/", + "https://m.twitch.tv/clip/clip/", +} + +// clips that are invalid due to path or domain+path combination +var invalidClips = []string{ + "https://clips.twitch.tv/pajlada/clip/VastBitterVultureMau5", + "https://clips.twitch.tv/", + "https://twitch.tv/nam____________________________________________/clip/someSlugNam", + "https://twitch.tv/supinic/clip/", + "https://twitch.tv/pajlada/clips/VastBitterVultureMau5", + "https://twitch.tv/zneix/clip/ImpossibleOilyAlpacaTF2John-jIlgtnSAQ52BThHhifyouseethisvivon", + "https://twitch.tv/clip/slug", + "https://gql.twitch.tv/VastBitterVultureMau5", + "https://gql.twitch.tv/ThreeLetterAPI/clip/VastBitterVultureMau5", + "https://m.twitch.tv/VastBitterVultureMau5", + "https://m.twitch.tv/username/clip/clip/slug", + "https://m.twitch.tv/username/notclip/slug", +} + +// clips that are invalid due to the path +var invalidClipSlugs = []string{ + "https://clips.twitch.tv/", + "https://twitch.tv/nam____________________________________________/clip/someSlugNam", + "https://twitch.tv/supinic/clip/", + "https://twitch.tv/pajlada/clips/VastBitterVultureMau5", + "https://twitch.tv/zneix/clip/ImpossibleOilyAlpacaTF2John-jIlgtnSAQ52BThHhifyouseethisvivon", + "https://m.twitch.tv/username/notclip/slug", +} diff --git a/internal/resolvers/twitch/initialize.go b/internal/resolvers/twitch/initialize.go index a861cb87..e444d015 100644 --- a/internal/resolvers/twitch/initialize.go +++ b/internal/resolvers/twitch/initialize.go @@ -42,7 +42,7 @@ var ( } ) -func Initialize(ctx context.Context, cfg config.APIConfig, pool db.Pool, helixClient *helix.Client, resolvers *[]resolver.Resolver) { +func Initialize(ctx context.Context, cfg config.APIConfig, pool db.Pool, helixClient TwitchAPIClient, resolvers *[]resolver.Resolver) { log := logger.FromContext(ctx) if helixClient == nil { log.Warnw("[Config] Twitch credentials missing, won't do special responses for Twitch") diff --git a/internal/resolvers/twitch/initialize_test.go b/internal/resolvers/twitch/initialize_test.go new file mode 100644 index 00000000..bc2894a4 --- /dev/null +++ b/internal/resolvers/twitch/initialize_test.go @@ -0,0 +1,39 @@ +package twitch + +import ( + "context" + "testing" + + "github.com/Chatterino/api/internal/logger" + "github.com/Chatterino/api/internal/mocks" + "github.com/Chatterino/api/pkg/config" + "github.com/Chatterino/api/pkg/resolver" + qt "github.com/frankban/quicktest" + "github.com/golang/mock/gomock" + "github.com/pashagolub/pgxmock" +) + +func TestInitialize(t *testing.T) { + ctx := logger.OnContext(context.Background(), logger.NewTest()) + c := qt.New(t) + ctrl := gomock.NewController(c) + helixClient := mocks.NewMockTwitchAPIClient(ctrl) + defer ctrl.Finish() + + cfg := config.APIConfig{} + pool, err := pgxmock.NewPool() + c.Assert(err, qt.IsNil) + + c.Run("No helix client", func(c *qt.C) { + customResolvers := []resolver.Resolver{} + c.Assert(customResolvers, qt.HasLen, 0) + Initialize(ctx, cfg, pool, nil, &customResolvers) + c.Assert(customResolvers, qt.HasLen, 0) + }) + c.Run("Helix client", func(c *qt.C) { + customResolvers := []resolver.Resolver{} + c.Assert(customResolvers, qt.HasLen, 0) + Initialize(ctx, cfg, pool, helixClient, &customResolvers) + c.Assert(customResolvers, qt.HasLen, 1) + }) +} diff --git a/internal/resolvers/twitch/parse_clip_slug_test.go b/internal/resolvers/twitch/parse_clip_slug_test.go index a3ceac5a..727f8193 100644 --- a/internal/resolvers/twitch/parse_clip_slug_test.go +++ b/internal/resolvers/twitch/parse_clip_slug_test.go @@ -4,24 +4,47 @@ import ( "net/url" "testing" + "github.com/Chatterino/api/pkg/utils" qt "github.com/frankban/quicktest" ) -func testParseClipSlug(urlString string) (string, error) { - u, err := url.Parse(urlString) - if err != nil { - return "", err +func TestParseClipSlug(t *testing.T) { + c := qt.New(t) + + type parseTest struct { + input *url.URL + expectedSlug string + expectedErr error } - return parseClipSlug(u) -} + tests := []parseTest{} -func TestParseClipSlug(t *testing.T) { - c := qt.New(t) + for _, b := range validClipBase { + tests = append(tests, parseTest{ + input: utils.MustParseURL(b + goodSlugV1), + expectedSlug: goodSlugV1, + expectedErr: nil, + }) + tests = append(tests, parseTest{ + input: utils.MustParseURL(b + goodSlugV2), + expectedSlug: goodSlugV2, + expectedErr: nil, + }) + } + + for _, c := range invalidClipSlugs { + tests = append(tests, parseTest{ + input: utils.MustParseURL(c), + expectedSlug: "", + expectedErr: errInvalidTwitchClip, + }) + } - for _, u := range validClips { - clipSlug, err := testParseClipSlug(u) - c.Assert(err, qt.IsNil, qt.Commentf("Valid clips must not error: %v", u)) - c.Assert([]string{goodSlugV1, goodSlugV2}, qt.Any(qt.Equals), clipSlug, qt.Commentf("%v must be seen as a clip", u)) + for _, test := range tests { + c.Run("", func(c *qt.C) { + clipSlug, err := parseClipSlug(test.input) + c.Assert(err, qt.Equals, test.expectedErr, qt.Commentf("%v", test.input)) + c.Assert(clipSlug, qt.Equals, test.expectedSlug, qt.Commentf("%v must be seen as a clip", test.input)) + }) } }