From 1e7d7071f2caa4ef537601a67c00ef858d573d9a Mon Sep 17 00:00:00 2001 From: James Scott Date: Wed, 20 Aug 2025 19:25:41 +0000 Subject: [PATCH 1/2] feat: Add version-based data parsing This commit introduces the capability to parse data files from GitHub releases based on their semantic version tag. This is crucial for handling evolving data schemas from upstream sources like `web-features`. Key changes: - **`lib/gh`**: `DownloadFileFromRelease` now returns a `*gh.File` struct containing both the file contents and the release's semver tag. - **`web_feature_consumer`**: A new `parseByVersion` method uses the release tag to select the appropriate parser. It currently supports routing to V2 and V3 parsers for `web-features` data, ensuring backward compatibility and readiness for the new schema. - **`bcd_consumer`**: Updated to use the new `*gh.File` struct, though it does not yet perform version-based parsing. - **Testing**: New tests have been added to validate the version-routing logic in the `web_feature_consumer`. --- lib/gh/download_file_from_release.go | 36 +++- lib/gh/download_file_from_release_test.go | 71 ++++++- lib/go.mod | 1 + lib/go.sum | 2 + .../pkg/workflow/bcd_releases_worker.go | 5 +- .../pkg/workflow/bcd_releases_worker_test.go | 28 ++- .../web_feature_consumer/cmd/job/main.go | 1 + .../pkg/workflow/web_features_worker.go | 86 +++++++-- .../pkg/workflow/web_features_worker_test.go | 181 ++++++++++++++++-- 9 files changed, 365 insertions(+), 46 deletions(-) diff --git a/lib/gh/download_file_from_release.go b/lib/gh/download_file_from_release.go index 69f39b22c..045bdecae 100644 --- a/lib/gh/download_file_from_release.go +++ b/lib/gh/download_file_from_release.go @@ -18,9 +18,12 @@ import ( "context" "errors" "io" + "log/slog" "net/http" + "strings" "github.com/google/go-github/v73/github" + "golang.org/x/mod/semver" ) var ( @@ -31,11 +34,21 @@ var ( ErrFatalError = errors.New("fatal error using github") ) +type File struct { + Contents io.ReadCloser + Info ReleaseInfo +} + +type ReleaseInfo struct { + // If the tag is valid, the will be non null. + Tag *string +} + func (c *Client) DownloadFileFromRelease( ctx context.Context, owner, repo string, httpClient *http.Client, - filePattern string) (io.ReadCloser, error) { + filePattern string) (*File, error) { release, _, err := c.repoClient.GetLatestRelease(ctx, owner, repo) if err != nil { // nolint: exhaustruct // WONTFIX. This is an external package. Cannot control it. @@ -84,5 +97,24 @@ func (c *Client) DownloadFileFromRelease( return nil, errors.Join(ErrUnableToDownloadAsset, err) } - return resp.Body, nil + // Returns a tag or empty string if not found. + tagName := release.GetTagName() + // In the event the tag is missing the prefix "v", add it. + // According https://pkg.go.dev/golang.org/x/mod/semver, the version must start with v + if len(tagName) > 0 && !strings.HasPrefix(tagName, "v") { + tagName = "v" + tagName + } + var tagNamePtr *string + if semver.IsValid(tagName) { + tagNamePtr = &tagName + } else { + slog.WarnContext(ctx, "invalid tag. it will not be used", "tag", tagName) + } + + return &File{ + Contents: resp.Body, + Info: ReleaseInfo{ + Tag: tagNamePtr, + }, + }, nil } diff --git a/lib/gh/download_file_from_release_test.go b/lib/gh/download_file_from_release_test.go index c2f34acd2..3128b9ba8 100644 --- a/lib/gh/download_file_from_release_test.go +++ b/lib/gh/download_file_from_release_test.go @@ -21,6 +21,7 @@ import ( "net/http" "testing" + "github.com/google/go-cmp/cmp" "github.com/google/go-github/v73/github" ) @@ -34,6 +35,12 @@ func checkIfFileIsReadable(t *testing.T, file io.Reader) { } } +func checkIfTagIsPopulated(t *testing.T, file *File) { + if file.Info.Tag == nil || *file.Info.Tag == "" { + t.Error("tag is empty") + } +} + func TestDownloadFileFromReleaseWebFeatures(t *testing.T) { t.Skip("Used for debugging purposes.") client := NewClient("") @@ -44,14 +51,14 @@ func TestDownloadFileFromReleaseWebFeatures(t *testing.T) { t.Errorf("unexpected error: %s", err.Error()) } else { // Close to be safe at the end. - defer file.Close() - checkIfFileIsReadable(t, file) + defer file.Contents.Close() + checkIfFileIsReadable(t, file.Contents) + checkIfTagIsPopulated(t, file) } } func TestDownloadFileFromReleaseBrowserCompatData(t *testing.T) { t.Skip("Used for debugging purposes.") - t.Skip("Cannot remove until https://github.com/mdn/browser-compat-data/issues/22675 is fixed") client := NewClient("") ctx := context.Background() httpClient := http.DefaultClient @@ -60,8 +67,9 @@ func TestDownloadFileFromReleaseBrowserCompatData(t *testing.T) { t.Errorf("unexpected error: %s", err.Error()) } else { // Close to be safe at the end. - defer file.Close() - checkIfFileIsReadable(t, file) + defer file.Contents.Close() + checkIfFileIsReadable(t, file.Contents) + checkIfTagIsPopulated(t, file) } } @@ -113,6 +121,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { cfg mockGetLatestReleaseConfig roundTripCfg *mockRoundTripperConfig expectedError error + expectedFile *File }{ { name: "successful download", @@ -127,6 +136,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { BrowserDownloadURL: valuePtr("http://example.com/file.txt"), }, }, + TagName: valuePtr("v1.0.0"), }, err: nil, }, @@ -135,10 +145,51 @@ func TestMockDownloadFileFromRelease(t *testing.T) { //nolint: exhaustruct resp: &http.Response{ StatusCode: http.StatusOK, + Body: io.NopCloser(nil), }, err: nil, }, expectedError: nil, + expectedFile: &File{ + Contents: io.NopCloser(nil), + Info: ReleaseInfo{ + Tag: valuePtr("v1.0.0"), + }, + }, + }, + { + name: "successful download without leading v in tag", + cfg: mockGetLatestReleaseConfig{ + expectedOwner: "owner", + expectedRepo: "repo", + //nolint: exhaustruct + release: &github.RepositoryRelease{ + Assets: []*github.ReleaseAsset{ + { + Name: valuePtr("file.txt"), + BrowserDownloadURL: valuePtr("http://example.com/file.txt"), + }, + }, + TagName: valuePtr("2.0.0"), + }, + err: nil, + }, + roundTripCfg: &mockRoundTripperConfig{ + expectedURL: "http://example.com/file.txt", + //nolint: exhaustruct + resp: &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(nil), + }, + err: nil, + }, + expectedError: nil, + expectedFile: &File{ + Contents: io.NopCloser(nil), + Info: ReleaseInfo{ + Tag: valuePtr("v2.0.0"), + }, + }, }, { name: "rate limit with github api", @@ -151,6 +202,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { }, roundTripCfg: nil, expectedError: ErrRateLimit, + expectedFile: nil, }, { name: "unknown error with github api", @@ -162,6 +214,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { }, roundTripCfg: nil, expectedError: ErrFatalError, + expectedFile: nil, }, { name: "missing asset", @@ -174,6 +227,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { }, roundTripCfg: nil, expectedError: ErrAssetNotFound, + expectedFile: nil, }, { name: "request.DO() fails", @@ -198,6 +252,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { err: errors.New("something went wrong"), }, expectedError: ErrUnableToDownloadAsset, + expectedFile: nil, }, { name: "failed to download", @@ -224,6 +279,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { err: nil, }, expectedError: ErrUnableToDownloadAsset, + expectedFile: nil, }, } for _, tc := range testCases { @@ -241,7 +297,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { httpClient := http.DefaultClient httpClient.Transport = &rt - _, err := client.DownloadFileFromRelease( + file, err := client.DownloadFileFromRelease( context.Background(), "owner", "repo", @@ -251,6 +307,9 @@ func TestMockDownloadFileFromRelease(t *testing.T) { if !errors.Is(err, tc.expectedError) { t.Errorf("unexpected error expected: %v received: %v", tc.expectedError, err) } + if diff := cmp.Diff(tc.expectedFile, file); diff != "" { + t.Errorf("unexpected file (-want +got):\n%s", diff) + } }) } } diff --git a/lib/go.mod b/lib/go.mod index 4acc21a3e..44cf3274a 100644 --- a/lib/go.mod +++ b/lib/go.mod @@ -68,6 +68,7 @@ require ( go.opentelemetry.io/otel/exporters/stdout/stdoutlog v0.13.0 // indirect go.opentelemetry.io/otel/log v0.13.0 // indirect go.opentelemetry.io/otel/sdk/log v0.13.0 // indirect + golang.org/x/mod v0.27.0 // indirect google.golang.org/appengine/v2 v2.0.6 // indirect ) diff --git a/lib/go.sum b/lib/go.sum index dfec48d99..a34195f85 100644 --- a/lib/go.sum +++ b/lib/go.sum @@ -1253,6 +1253,8 @@ golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91 golang.org/x/mod v0.7.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.9.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= +golang.org/x/mod v0.27.0 h1:kb+q2PyFnEADO2IEF935ehFUXlWiNjJWtRNgBLSfbxQ= +golang.org/x/mod v0.27.0/go.mod h1:rWI627Fq0DEoudcK+MBkNkCe0EetEaDSwJJkCcjpazc= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= diff --git a/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker.go b/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker.go index f16d6f884..1c1a02591 100644 --- a/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker.go +++ b/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker.go @@ -20,6 +20,7 @@ import ( "net/http" "github.com/GoogleChrome/webstatus.dev/lib/gcpspanner/spanneradapters/bcdconsumertypes" + "github.com/GoogleChrome/webstatus.dev/lib/gh" "github.com/GoogleChrome/webstatus.dev/workflows/steps/services/bcd_consumer/pkg/data" ) @@ -45,7 +46,7 @@ type DataGetter interface { ctx context.Context, owner, repo string, httpClient *http.Client, - filePattern string) (io.ReadCloser, error) + filePattern string) (*gh.File, error) } // DataParser describes the behavior to read raw bytes into the expected BCDData struct. @@ -106,7 +107,7 @@ func (p BCDJobProcessor) Process( } // Step 2. Parse the file. - data, err := p.dataParser.Parse(file) + data, err := p.dataParser.Parse(file.Contents) if err != nil { return err } diff --git a/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker_test.go b/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker_test.go index 1dfbe61bf..c0c3b05a8 100644 --- a/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker_test.go +++ b/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker_test.go @@ -27,6 +27,7 @@ import ( "github.com/GoogleChrome/webstatus.dev/lib/gcpspanner/spanneradapters/bcdconsumertypes" "github.com/GoogleChrome/webstatus.dev/lib/gen/jsonschema/mdn__browser_compat_data" + "github.com/GoogleChrome/webstatus.dev/lib/gh" "github.com/GoogleChrome/webstatus.dev/workflows/steps/services/bcd_consumer/pkg/data" ) @@ -56,7 +57,7 @@ type mockDownloadFileFromReleaseConfig struct { repoOwner string repoName string filePattern string - fakeFile io.ReadCloser + fakeFile *gh.File err error } @@ -69,7 +70,7 @@ func (m *MockDataGetter) DownloadFileFromRelease( _ context.Context, owner, repo string, _ *http.Client, - filePattern string) (io.ReadCloser, error) { + filePattern string) (*gh.File, error) { if m.mockDownloadFileFromReleaseCfg.repoOwner != owner || m.mockDownloadFileFromReleaseCfg.repoName != repo || m.mockDownloadFileFromReleaseCfg.filePattern != filePattern { @@ -97,7 +98,8 @@ func (m *MockDataParser) Parse(in io.ReadCloser) (*data.BCDData, error) { m.t.Errorf("unable to read file") } if m.mockParseCfg.expectedFileContents != string(fileContents) { - m.t.Error("unexpected file contents") + m.t.Errorf("unexpected file contents. want: %s, got: %s", + m.mockParseCfg.expectedFileContents, string(fileContents)) } return m.mockParseCfg.ret, m.mockParseCfg.err @@ -205,6 +207,16 @@ func getSampleReleases() []bcdconsumertypes.BrowserRelease { } func TestProcess(t *testing.T) { + // Create a function to generate a file because Contents can only be read once + testFileFn := func() *gh.File { + return &gh.File{ + Contents: io.NopCloser(strings.NewReader("success")), + Info: gh.ReleaseInfo{ + Tag: nil, + }, + } + } + testCases := []processWorkflowTest{ { name: "successful process", @@ -215,7 +227,7 @@ func TestProcess(t *testing.T) { repoOwner: repoOwner, repoName: repoName, filePattern: filePattern, - fakeFile: io.NopCloser(strings.NewReader("success")), + fakeFile: testFileFn(), err: nil, }, mockParseCfg: &mockParseConfig{ @@ -244,7 +256,7 @@ func TestProcess(t *testing.T) { repoOwner: repoOwner, repoName: repoName, filePattern: filePattern, - fakeFile: io.NopCloser(strings.NewReader("success")), + fakeFile: testFileFn(), err: errTestGetter, }, mockParseCfg: nil, @@ -261,7 +273,7 @@ func TestProcess(t *testing.T) { repoOwner: repoOwner, repoName: repoName, filePattern: filePattern, - fakeFile: io.NopCloser(strings.NewReader("success")), + fakeFile: testFileFn(), err: nil, }, mockParseCfg: &mockParseConfig{ @@ -282,7 +294,7 @@ func TestProcess(t *testing.T) { repoOwner: repoOwner, repoName: repoName, filePattern: filePattern, - fakeFile: io.NopCloser(strings.NewReader("success")), + fakeFile: testFileFn(), err: nil, }, mockParseCfg: &mockParseConfig{ @@ -308,7 +320,7 @@ func TestProcess(t *testing.T) { repoOwner: repoOwner, repoName: repoName, filePattern: filePattern, - fakeFile: io.NopCloser(strings.NewReader("success")), + fakeFile: testFileFn(), err: nil, }, mockParseCfg: &mockParseConfig{ diff --git a/workflows/steps/services/web_feature_consumer/cmd/job/main.go b/workflows/steps/services/web_feature_consumer/cmd/job/main.go index 8d04bed55..65a225837 100644 --- a/workflows/steps/services/web_feature_consumer/cmd/job/main.go +++ b/workflows/steps/services/web_feature_consumer/cmd/job/main.go @@ -92,6 +92,7 @@ func main() { spanneradapters.NewWebFeatureGroupsConsumer(spannerClient), spanneradapters.NewWebFeatureSnapshotsConsumer(spannerClient), data.Parser{}, + data.V3Parser{}, ) // Job Generation diff --git a/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker.go b/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker.go index 8ed504fda..56ae7429f 100644 --- a/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker.go +++ b/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker.go @@ -16,13 +16,16 @@ package workflow import ( "context" + "errors" "io" "log/slog" "net/http" "time" "github.com/GoogleChrome/webstatus.dev/lib/gen/jsonschema/web_platform_dx__web_features" + "github.com/GoogleChrome/webstatus.dev/lib/gh" "github.com/GoogleChrome/webstatus.dev/lib/webdxfeaturetypes" + "golang.org/x/mod/semver" ) // AssetGetter describes the behavior to get a certain asset from a github release. @@ -31,7 +34,7 @@ type AssetGetter interface { ctx context.Context, owner, repo string, httpClient *http.Client, - filePattern string) (io.ReadCloser, error) + filePattern string) (*gh.File, error) } // AssetParser describes the behavior to parse the io.ReadCloser from AssetGetter into the expected data type. @@ -39,6 +42,11 @@ type AssetParser interface { Parse(io.ReadCloser) (*webdxfeaturetypes.ProcessedWebFeaturesData, error) } +var ( + ErrUnknownAssetVersion = errors.New("unknown asset version") + ErrUnsupportedAssetVersion = errors.New("unsupported asset version") +) + // WebFeatureStorer describes the logic to insert the web features that were returned by the AssetParser. type WebFeatureStorer interface { InsertWebFeatures( @@ -86,25 +94,73 @@ func NewWebFeaturesJobProcessor(assetGetter AssetGetter, metadataStorer WebFeatureMetadataStorer, groupStorer WebDXGroupStorer, snapshotStorer WebDXSnapshotStorer, - webFeaturesDataParser AssetParser, + webFeaturesDataV2Parser AssetParser, + webFeaturesDataV3Parser AssetParser, ) WebFeaturesJobProcessor { return WebFeaturesJobProcessor{ - assetGetter: assetGetter, - storer: storer, - metadataStorer: metadataStorer, - groupStorer: groupStorer, - snapshotStorer: snapshotStorer, - webFeaturesDataParser: webFeaturesDataParser, + assetGetter: assetGetter, + storer: storer, + metadataStorer: metadataStorer, + groupStorer: groupStorer, + snapshotStorer: snapshotStorer, + webFeaturesDataV2Parser: webFeaturesDataV2Parser, + webFeaturesDataV3Parser: webFeaturesDataV3Parser, } } type WebFeaturesJobProcessor struct { - assetGetter AssetGetter - storer WebFeatureStorer - metadataStorer WebFeatureMetadataStorer - groupStorer WebDXGroupStorer - snapshotStorer WebDXSnapshotStorer - webFeaturesDataParser AssetParser + assetGetter AssetGetter + storer WebFeatureStorer + metadataStorer WebFeatureMetadataStorer + groupStorer WebDXGroupStorer + snapshotStorer WebDXSnapshotStorer + webFeaturesDataV2Parser AssetParser + webFeaturesDataV3Parser AssetParser +} + +const ( + // According to https://pkg.go.dev/golang.org/x/mod/semver, the version must start with "v". + v2 = "v2.0.0" + v3 = "v3.0.0" + v4 = "v4.0.0" +) + +func (p WebFeaturesJobProcessor) parseByVersion(ctx context.Context, file *gh.File) ( + *webdxfeaturetypes.ProcessedWebFeaturesData, error) { + if file.Info.Tag == nil { + slog.ErrorContext(ctx, "unknown version", "version", "nil") + + return nil, ErrUnknownAssetVersion + } + + if semver.Compare(*file.Info.Tag, v3) == -1 { + // If less than version 3, use default v2 parser + slog.InfoContext(ctx, "using v2 parser", "version", *file.Info.Tag) + data, err := p.webFeaturesDataV2Parser.Parse(file.Contents) + if err != nil { + slog.ErrorContext(ctx, "unable to parse v2 data", "error", err) + + return nil, err + } + + return data, nil + + } else if semver.Compare(*file.Info.Tag, v4) == -1 { + // If version 3, use v3 parser + slog.InfoContext(ctx, "using v3 parser", "version", *file.Info.Tag) + data, err := p.webFeaturesDataV3Parser.Parse(file.Contents) + if err != nil { + slog.ErrorContext(ctx, "unable to parse v3 data", "error", err) + + return nil, err + } + + return data, nil + } + + slog.ErrorContext(ctx, "unsupported version", "version", *file.Info.Tag) + + return nil, ErrUnsupportedAssetVersion } func (p WebFeaturesJobProcessor) Process(ctx context.Context, job JobArguments) error { @@ -120,7 +176,7 @@ func (p WebFeaturesJobProcessor) Process(ctx context.Context, job JobArguments) return err } - data, err := p.webFeaturesDataParser.Parse(file) + data, err := p.parseByVersion(ctx, file) if err != nil { slog.ErrorContext(ctx, "unable to parse data", "error", err) diff --git a/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker_test.go b/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker_test.go index 46c403ba6..46d642d09 100644 --- a/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker_test.go +++ b/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker_test.go @@ -25,6 +25,7 @@ import ( "time" "github.com/GoogleChrome/webstatus.dev/lib/gen/jsonschema/web_platform_dx__web_features" + "github.com/GoogleChrome/webstatus.dev/lib/gh" "github.com/GoogleChrome/webstatus.dev/lib/webdxfeaturetypes" ) @@ -48,24 +49,25 @@ type mockDownloadFileFromReleaseConfig struct { expectedFileName string expectedOwner string expectedRepo string - returnReadCloser io.ReadCloser + returnFile *gh.File returnError error } func (m *mockAssetGetter) DownloadFileFromRelease( - _ context.Context, owner, repo string, _ *http.Client, filePattern string) (io.ReadCloser, error) { + _ context.Context, owner, repo string, _ *http.Client, filePattern string) (*gh.File, error) { if filePattern != m.mockDownloadFileFromReleaseCfg.expectedFileName || owner != m.mockDownloadFileFromReleaseCfg.expectedOwner || repo != m.mockDownloadFileFromReleaseCfg.expectedRepo { m.t.Error("unexpected input to DownloadFileFromRelease") } - return m.mockDownloadFileFromReleaseCfg.returnReadCloser, m.mockDownloadFileFromReleaseCfg.returnError + return m.mockDownloadFileFromReleaseCfg.returnFile, m.mockDownloadFileFromReleaseCfg.returnError } type mockAssetParser struct { t *testing.T mockParseCfg mockParseConfig + callCount int } type mockParseConfig struct { @@ -75,13 +77,15 @@ type mockParseConfig struct { } func (m *mockAssetParser) Parse(file io.ReadCloser) (*webdxfeaturetypes.ProcessedWebFeaturesData, error) { + m.callCount++ defer file.Close() fileContents, err := io.ReadAll(file) if err != nil { m.t.Errorf("unable to read file") } if string(fileContents) != m.mockParseCfg.expectedFileContents { - m.t.Error("unexpected file contents") + m.t.Errorf("unexpected file contents want: %s, got: %s", + m.mockParseCfg.expectedFileContents, string(fileContents)) } return m.mockParseCfg.returnData, m.mockParseCfg.returnError @@ -258,7 +262,20 @@ const ( testFileName = "file.txt" ) +func testFile(tag *string, contents string) *gh.File { + return &gh.File{ + Contents: io.NopCloser(strings.NewReader(contents)), + Info: gh.ReleaseInfo{ + Tag: tag, + }, + } +} + func TestProcess(t *testing.T) { + // According https://pkg.go.dev/golang.org/x/mod/semver, the version must start with v + testFileFn := func() *gh.File { + return testFile(valuePtr(v2), "hi features") + } // nolint: dupl testCases := []struct { name string @@ -278,7 +295,7 @@ func TestProcess(t *testing.T) { expectedOwner: testRepoOwner, expectedRepo: testRepoName, expectedFileName: testFileName, - returnReadCloser: io.NopCloser(strings.NewReader("hi features")), + returnFile: testFileFn(), returnError: nil, }, mockParseCfg: mockParseConfig{ @@ -519,7 +536,7 @@ func TestProcess(t *testing.T) { expectedOwner: testRepoOwner, expectedRepo: testRepoName, expectedFileName: testFileName, - returnReadCloser: io.NopCloser(strings.NewReader("hi features")), + returnFile: testFileFn(), returnError: errTestFailToGetAsset, }, mockParseCfg: mockParseConfig{ @@ -558,7 +575,7 @@ func TestProcess(t *testing.T) { expectedOwner: testRepoOwner, expectedRepo: testRepoName, expectedFileName: testFileName, - returnReadCloser: io.NopCloser(strings.NewReader("hi features")), + returnFile: testFileFn(), returnError: nil, }, mockParseCfg: mockParseConfig{ @@ -597,7 +614,7 @@ func TestProcess(t *testing.T) { expectedOwner: testRepoOwner, expectedRepo: testRepoName, expectedFileName: testFileName, - returnReadCloser: io.NopCloser(strings.NewReader("hi features")), + returnFile: testFileFn(), returnError: nil, }, mockParseCfg: mockParseConfig{ @@ -701,7 +718,7 @@ func TestProcess(t *testing.T) { expectedOwner: testRepoOwner, expectedRepo: testRepoName, expectedFileName: testFileName, - returnReadCloser: io.NopCloser(strings.NewReader("hi features")), + returnFile: testFileFn(), returnError: nil, }, mockParseCfg: mockParseConfig{ @@ -834,7 +851,7 @@ func TestProcess(t *testing.T) { expectedOwner: testRepoOwner, expectedRepo: testRepoName, expectedFileName: testFileName, - returnReadCloser: io.NopCloser(strings.NewReader("hi features")), + returnFile: testFileFn(), returnError: nil, }, mockParseCfg: mockParseConfig{ @@ -1004,7 +1021,7 @@ func TestProcess(t *testing.T) { expectedOwner: testRepoOwner, expectedRepo: testRepoName, expectedFileName: testFileName, - returnReadCloser: io.NopCloser(strings.NewReader("hi features")), + returnFile: testFileFn(), returnError: nil, }, mockParseCfg: mockParseConfig{ @@ -1213,7 +1230,7 @@ func TestProcess(t *testing.T) { expectedOwner: testRepoOwner, expectedRepo: testRepoName, expectedFileName: testFileName, - returnReadCloser: io.NopCloser(strings.NewReader("hi features")), + returnFile: testFileFn(), returnError: nil, }, mockParseCfg: mockParseConfig{ @@ -1443,7 +1460,7 @@ func TestProcess(t *testing.T) { expectedOwner: testRepoOwner, expectedRepo: testRepoName, expectedFileName: testFileName, - returnReadCloser: io.NopCloser(strings.NewReader("hi features")), + returnFile: testFileFn(), returnError: nil, }, mockParseCfg: mockParseConfig{ @@ -1688,6 +1705,12 @@ func TestProcess(t *testing.T) { mockParser := &mockAssetParser{ t: t, mockParseCfg: tc.mockParseCfg, + callCount: 0, + } + mockParserV3 := &mockAssetParser{ + t: t, + mockParseCfg: tc.mockParseCfg, + callCount: 0, } mockStorer := &mockWebFeatureStorer{ t: t, @@ -1717,6 +1740,7 @@ func TestProcess(t *testing.T) { mockGroupStorer, mockSnapshotStorer, mockParser, + mockParserV3, ) err := processor.Process(context.TODO(), NewJobArguments( @@ -1732,3 +1756,134 @@ func TestProcess(t *testing.T) { }) } } + +func valuePtr[T any](in T) *T { return &in } + +func TestParseByVersion(t *testing.T) { + testCases := []struct { + name string + file *gh.File + v2Parser *mockAssetParser + expectedV2CallCount int + v3Parser *mockAssetParser + expectedV3CallCount int + expectedError error + }{ + { + name: "missing tag", + file: testFile(nil, ""), + v2Parser: nil, + v3Parser: nil, + expectedError: ErrUnknownAssetVersion, + expectedV2CallCount: 0, + expectedV3CallCount: 0, + }, + { + name: "v2 parses successfully", + file: testFile(valuePtr(v2), ""), + v2Parser: &mockAssetParser{ + t: t, + mockParseCfg: mockParseConfig{ + expectedFileContents: "", + returnError: nil, + returnData: nil, + }, + callCount: 0, + }, + v3Parser: nil, + expectedError: nil, + expectedV2CallCount: 1, + expectedV3CallCount: 0, + }, + { + name: "v2 parses unsuccessfully", + file: testFile(valuePtr(v2), ""), + v2Parser: &mockAssetParser{ + t: t, + mockParseCfg: mockParseConfig{ + expectedFileContents: "", + returnError: errTestCannotParseData, + returnData: nil, + }, + callCount: 0, + }, + v3Parser: nil, + expectedError: errTestCannotParseData, + expectedV2CallCount: 1, + expectedV3CallCount: 0, + }, + { + name: "v3 parses successfully", + file: testFile(valuePtr(v3), ""), + v2Parser: nil, + v3Parser: &mockAssetParser{ + t: t, + mockParseCfg: mockParseConfig{ + expectedFileContents: "", + returnError: nil, + returnData: nil, + }, + callCount: 0, + }, + expectedError: nil, + expectedV2CallCount: 0, + expectedV3CallCount: 1, + }, + { + name: "v3 parses unsuccessfully", + file: testFile(valuePtr(v3), ""), + v2Parser: nil, + v3Parser: &mockAssetParser{ + t: t, + mockParseCfg: mockParseConfig{ + expectedFileContents: "", + returnError: errTestCannotParseData, + returnData: nil, + }, + callCount: 0, + }, + expectedError: errTestCannotParseData, + expectedV2CallCount: 0, + expectedV3CallCount: 1, + }, + { + name: "unsupported tag", + file: testFile(valuePtr("v4.0.0"), ""), + v2Parser: nil, + v3Parser: nil, + expectedError: ErrUnsupportedAssetVersion, + expectedV2CallCount: 0, + expectedV3CallCount: 0, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + p := WebFeaturesJobProcessor{ + assetGetter: nil, + webFeaturesDataV2Parser: tc.v2Parser, + webFeaturesDataV3Parser: tc.v3Parser, + storer: nil, + metadataStorer: nil, + groupStorer: nil, + snapshotStorer: nil, + } + _, err := p.parseByVersion(t.Context(), tc.file) + if !errors.Is(err, tc.expectedError) { + t.Errorf("Expected error: %v, Got: %v", tc.expectedError, err) + } + if tc.v2Parser != nil && tc.v2Parser.callCount != tc.expectedV2CallCount { + t.Errorf("Expected v2 call count: %d, Got: %d", tc.expectedV2CallCount, tc.v2Parser.callCount) + } + if tc.v2Parser == nil && tc.expectedV2CallCount > 0 { + t.Error("Expected v2 parser to be called") + } + if tc.v3Parser != nil && tc.v3Parser.callCount != tc.expectedV3CallCount { + t.Errorf("Expected v3 call count: %d, Got: %d", tc.expectedV3CallCount, tc.v3Parser.callCount) + } + if tc.v3Parser == nil && tc.expectedV3CallCount > 0 { + t.Error("Expected v3 parser to be called") + } + }) + } +} From 5c6fa87ac716368b87ad983f29f6e16167db1c9e Mon Sep 17 00:00:00 2001 From: James Scott Date: Fri, 22 Aug 2025 17:59:31 +0000 Subject: [PATCH 2/2] address feedback --- lib/gh/download_file_from_release.go | 7 ++++--- lib/gh/download_file_from_release_test.go | 8 ++++---- .../bcd_consumer/pkg/workflow/bcd_releases_worker.go | 2 +- .../pkg/workflow/bcd_releases_worker_test.go | 8 ++++---- .../pkg/workflow/web_features_worker.go | 4 ++-- .../pkg/workflow/web_features_worker_test.go | 12 ++++++------ 6 files changed, 21 insertions(+), 20 deletions(-) diff --git a/lib/gh/download_file_from_release.go b/lib/gh/download_file_from_release.go index 045bdecae..8fd6c4ec1 100644 --- a/lib/gh/download_file_from_release.go +++ b/lib/gh/download_file_from_release.go @@ -34,7 +34,8 @@ var ( ErrFatalError = errors.New("fatal error using github") ) -type File struct { +// ReleaseFile represents a file in a given Github release. +type ReleaseFile struct { Contents io.ReadCloser Info ReleaseInfo } @@ -48,7 +49,7 @@ func (c *Client) DownloadFileFromRelease( ctx context.Context, owner, repo string, httpClient *http.Client, - filePattern string) (*File, error) { + filePattern string) (*ReleaseFile, error) { release, _, err := c.repoClient.GetLatestRelease(ctx, owner, repo) if err != nil { // nolint: exhaustruct // WONTFIX. This is an external package. Cannot control it. @@ -111,7 +112,7 @@ func (c *Client) DownloadFileFromRelease( slog.WarnContext(ctx, "invalid tag. it will not be used", "tag", tagName) } - return &File{ + return &ReleaseFile{ Contents: resp.Body, Info: ReleaseInfo{ Tag: tagNamePtr, diff --git a/lib/gh/download_file_from_release_test.go b/lib/gh/download_file_from_release_test.go index 3128b9ba8..fe3d19d71 100644 --- a/lib/gh/download_file_from_release_test.go +++ b/lib/gh/download_file_from_release_test.go @@ -35,7 +35,7 @@ func checkIfFileIsReadable(t *testing.T, file io.Reader) { } } -func checkIfTagIsPopulated(t *testing.T, file *File) { +func checkIfTagIsPopulated(t *testing.T, file *ReleaseFile) { if file.Info.Tag == nil || *file.Info.Tag == "" { t.Error("tag is empty") } @@ -121,7 +121,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { cfg mockGetLatestReleaseConfig roundTripCfg *mockRoundTripperConfig expectedError error - expectedFile *File + expectedFile *ReleaseFile }{ { name: "successful download", @@ -150,7 +150,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { err: nil, }, expectedError: nil, - expectedFile: &File{ + expectedFile: &ReleaseFile{ Contents: io.NopCloser(nil), Info: ReleaseInfo{ Tag: valuePtr("v1.0.0"), @@ -184,7 +184,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) { err: nil, }, expectedError: nil, - expectedFile: &File{ + expectedFile: &ReleaseFile{ Contents: io.NopCloser(nil), Info: ReleaseInfo{ Tag: valuePtr("v2.0.0"), diff --git a/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker.go b/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker.go index 1c1a02591..96c3052e9 100644 --- a/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker.go +++ b/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker.go @@ -46,7 +46,7 @@ type DataGetter interface { ctx context.Context, owner, repo string, httpClient *http.Client, - filePattern string) (*gh.File, error) + filePattern string) (*gh.ReleaseFile, error) } // DataParser describes the behavior to read raw bytes into the expected BCDData struct. diff --git a/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker_test.go b/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker_test.go index c0c3b05a8..dd6eeb384 100644 --- a/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker_test.go +++ b/workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker_test.go @@ -57,7 +57,7 @@ type mockDownloadFileFromReleaseConfig struct { repoOwner string repoName string filePattern string - fakeFile *gh.File + fakeFile *gh.ReleaseFile err error } @@ -70,7 +70,7 @@ func (m *MockDataGetter) DownloadFileFromRelease( _ context.Context, owner, repo string, _ *http.Client, - filePattern string) (*gh.File, error) { + filePattern string) (*gh.ReleaseFile, error) { if m.mockDownloadFileFromReleaseCfg.repoOwner != owner || m.mockDownloadFileFromReleaseCfg.repoName != repo || m.mockDownloadFileFromReleaseCfg.filePattern != filePattern { @@ -208,8 +208,8 @@ func getSampleReleases() []bcdconsumertypes.BrowserRelease { func TestProcess(t *testing.T) { // Create a function to generate a file because Contents can only be read once - testFileFn := func() *gh.File { - return &gh.File{ + testFileFn := func() *gh.ReleaseFile { + return &gh.ReleaseFile{ Contents: io.NopCloser(strings.NewReader("success")), Info: gh.ReleaseInfo{ Tag: nil, diff --git a/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker.go b/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker.go index 56ae7429f..b6f6a8725 100644 --- a/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker.go +++ b/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker.go @@ -34,7 +34,7 @@ type AssetGetter interface { ctx context.Context, owner, repo string, httpClient *http.Client, - filePattern string) (*gh.File, error) + filePattern string) (*gh.ReleaseFile, error) } // AssetParser describes the behavior to parse the io.ReadCloser from AssetGetter into the expected data type. @@ -125,7 +125,7 @@ const ( v4 = "v4.0.0" ) -func (p WebFeaturesJobProcessor) parseByVersion(ctx context.Context, file *gh.File) ( +func (p WebFeaturesJobProcessor) parseByVersion(ctx context.Context, file *gh.ReleaseFile) ( *webdxfeaturetypes.ProcessedWebFeaturesData, error) { if file.Info.Tag == nil { slog.ErrorContext(ctx, "unknown version", "version", "nil") diff --git a/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker_test.go b/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker_test.go index 46d642d09..65f5ea224 100644 --- a/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker_test.go +++ b/workflows/steps/services/web_feature_consumer/pkg/workflow/web_features_worker_test.go @@ -49,12 +49,12 @@ type mockDownloadFileFromReleaseConfig struct { expectedFileName string expectedOwner string expectedRepo string - returnFile *gh.File + returnFile *gh.ReleaseFile returnError error } func (m *mockAssetGetter) DownloadFileFromRelease( - _ context.Context, owner, repo string, _ *http.Client, filePattern string) (*gh.File, error) { + _ context.Context, owner, repo string, _ *http.Client, filePattern string) (*gh.ReleaseFile, error) { if filePattern != m.mockDownloadFileFromReleaseCfg.expectedFileName || owner != m.mockDownloadFileFromReleaseCfg.expectedOwner || repo != m.mockDownloadFileFromReleaseCfg.expectedRepo { @@ -262,8 +262,8 @@ const ( testFileName = "file.txt" ) -func testFile(tag *string, contents string) *gh.File { - return &gh.File{ +func testFile(tag *string, contents string) *gh.ReleaseFile { + return &gh.ReleaseFile{ Contents: io.NopCloser(strings.NewReader(contents)), Info: gh.ReleaseInfo{ Tag: tag, @@ -273,7 +273,7 @@ func testFile(tag *string, contents string) *gh.File { func TestProcess(t *testing.T) { // According https://pkg.go.dev/golang.org/x/mod/semver, the version must start with v - testFileFn := func() *gh.File { + testFileFn := func() *gh.ReleaseFile { return testFile(valuePtr(v2), "hi features") } // nolint: dupl @@ -1762,7 +1762,7 @@ func valuePtr[T any](in T) *T { return &in } func TestParseByVersion(t *testing.T) { testCases := []struct { name string - file *gh.File + file *gh.ReleaseFile v2Parser *mockAssetParser expectedV2CallCount int v3Parser *mockAssetParser