Add change calculation tool #1975

denik · 2024-12-06T15:17:34Z

Changes

Add a new tool changecalc that calculates all packages that need to be re-tested given a list of changed files. Without a list, uses git to compare with main branch.

'make viewchanges' runs the tool.
'make testchanges' runs the test suite over affected packages.

New tests-changes github job is added in shadow mode that takes advantage of this tool.

Tests

Manually. If we land this, we can observe the job on real PRs and how it compares with full runs.

Add a new tool changecalc that calculates all packages that need to be re-tested given a list of changed files. Without a list, uses git to compare with main branch. 'make viewchanges' runs the tool. 'make testchanges' runs the test suite over affected packages. New tests-changes github job is added in shadow mode that takes advantage of this tool.

denik · 2024-12-06T15:34:06Z

Example test:

Checkout this branch
Modify changecalc.yml to point to this branch
Modify bundle/config/mutator/load_git_details.go

% git diff
diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go
index 82255552..fe4a2639 100644
--- a/bundle/config/mutator/load_git_details.go
+++ b/bundle/config/mutator/load_git_details.go
@@ -21,6 +21,7 @@ func (m *loadGitDetails) Name() string {
 }

 func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
+
        var diags diag.Diagnostics
        info, err := git.FetchRepositoryInfo(ctx, b.BundleRoot.Native(), b.WorkspaceClient())
        if err != nil {
diff --git a/changecalc.yml b/changecalc.yml
index d4586f44..d95ae7fb 100644
--- a/changecalc.yml
+++ b/changecalc.yml
@@ -1,4 +1,4 @@
-base_branch: main
+base_branch: denis.bilenko/changecalc
 go_mod: go.mod
 reset_list:
   - go.mod

Use 'make viewchanges' to preview affected packages

% make viewchanges | wc -l                                                                                                                                 denis.bilenko/changecalc*
     148

Run unit tests over affected packages only:

% make testchanges | tail -n 1
DONE 821 tests, 265 skipped in 9.188s

Compare with running all unit tests:

% make test | tail -n 1
DONE 2318 tests, 280 skipped in 24.037s

Run make testchanges cat: changed-packages.txt: No such file or directory ✓ Running tests based on changes relative to main... changecalc/changecalc > changed-packages.txt || echo "./..." > changed-packages.txt fatal: bad revision 'main' failed to execute [git diff main --name-only -- .]: exit status 128 gotestsum --format pkgname-and-test-fails --no-summary=skipped --raw-command go test -v -json -short -coverprofile=coverage.txt ✓ . (32ms) (coverage: 0.0% of statements)

Getting this error changecalc/changecalc > changed-packages.txt || echo "./..." > changed-packages.txt fatal: bad revision 'main' failed to execute [git diff main --name-only -- .]: exit status 128 https://stackoverflow.com/a/76769122

as recommended here https://github.com/orgs/community/discussions/25797#discussioncomment-3249294

recommended here actions/checkout#118

This reverts commit 7954f92.

github-actions · 2024-12-06T16:27:50Z

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

PR number: 1975
Commit SHA: 41cc96c0fc0dc3f358dae0a83c8651ff46f4b164

Checks will be approved automatically on success.

eng-dev-ecosystem-bot · 2024-12-06T16:28:30Z

Test Details: go/deco-tests/12202420953

shreyas-goenka

Thanks! This would be nice to have for local development but I'm not sure we can use this for our unit testing workflow since there might be implicit dependencies that are not captured by the import dependencies.

Could you clarify the intention behind adding this functionality?

shreyas-goenka · 2024-12-09T10:02:04Z

.github/workflows/push.yml

+          - windows-latest
+
+    steps:
+      - name: Checkout repository and submodules


Create a reusable workflow for the first 5 steps since they are the same for both all and partial unit tests?
https://dev.to/n3wt0n/avoid-duplication-github-actions-reusable-workflows-3ae8

The command to run can be passed as an arg to the common workflow.

The intent is to replace the existing one with this one, so we can keep the dup for a little while.

shreyas-goenka · 2024-12-09T10:04:48Z

.github/workflows/push.yml

@@ -57,6 +57,57 @@ jobs:
      - name: Publish test coverage
        uses: codecov/codecov-action@v4

+  tests-changes:


I presume the intention behind this is to facilitate faster local development since now you can simply run make testchanges? For the push workflow we would still need to run everything since import dependencies are not enough to track all relevant tests.

Could you add a comment here explaining the intention of running this as a workflow? Presumably to ensure the test changes section works correctly?

The intention for this tool to be used for running both unit tests and integration tests on CI. It can save significant time, especially in this cases:

test only change; in that case

change is local affects only set of tests (e.g. bundle tests).

In that case we won't need 2 workflows, only workflow via this tool will remain. Engineers will be able to opt out, e.g. by adding /fulltests in description or comment.

For the push workflow we would still need to run everything since import dependencies are not enough to track all relevant tests.

Can you give counter-example? Note, it also establishes data dependencies (with some assumptions).

shreyas-goenka · 2024-12-09T10:09:00Z

changecalc/main.go

changecalc can be a package in internal instead of a top-level CLI package.

+1, or "tools". I'm sure there are more we'd like to check in and take advantage of.

As long as it is not top-level.

shreyas-goenka · 2024-12-09T10:12:23Z

changecalc/main.go

+// GetChangedFiles compares the current branch to the base branch
+// and returns a slice of file paths that have been modified.
+func GetChangedFiles(baseBranch string) ([]string, error) {
+	command := []string{"git", "diff", baseBranch, "--name-only", "--", "."}


Should we use git status here if this is meant to facilitate local development?

Not sure what you mean? Could you clarify how "git status" would be used?

pietern

Looks cool!

pietern · 2024-12-09T21:58:00Z

.github/workflows/push.yml

+          - windows-latest
+
+    steps:
+      - name: Checkout repository and submodules


The intent is to replace the existing one with this one, so we can keep the dup for a little while.

pietern · 2024-12-09T21:58:59Z

Makefile

+	xargs gotestsum --format pkgname-and-test-fails --no-summary=skipped --raw-command go test -v -json -short -coverprofile=coverage.txt < affected-packages.txt
+
+changecalc/changecalc: changecalc/*.go
+	@go build -o changecalc/changecalc changecalc/main.go


I prefer to not have targets for Go builds if we can rely on go run to do the caching and invalidation for us.

pietern · 2024-12-09T21:59:34Z

changecalc/main.go

+1, or "tools". I'm sure there are more we'd like to check in and take advantage of.

As long as it is not top-level.

pietern · 2024-12-09T22:00:13Z

changecalc.yml

@@ -0,0 +1,6 @@
+base_branch: refs/remotes/origin/main


Do we need to configure this in a config file?

pietern · 2024-12-09T22:04:47Z

changecalc/main.go

+		}
+	}
+	return false
+}


Some of these functions could move into a dedicated and testable package.

denik temporarily deployed to test-trigger-is December 6, 2024 15:17 — with GitHub Actions Inactive

denik requested a review from pietern December 6, 2024 15:18

denik force-pushed the denis.bilenko/changecalc branch from a081a1e to b0c4645 Compare December 6, 2024 15:27

denik temporarily deployed to test-trigger-is December 6, 2024 15:27 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is December 6, 2024 15:28 — with GitHub Actions Inactive

denik requested review from andrewnester and shreyas-goenka December 6, 2024 15:34

fix workflow

024a43f

denik temporarily deployed to test-trigger-is December 6, 2024 15:40 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is December 6, 2024 15:41 — with GitHub Actions Inactive

use yaml.v3

0ff74ba

denik temporarily deployed to test-trigger-is December 6, 2024 15:47 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is December 6, 2024 15:53 — with GitHub Actions Inactive

add fetch-depth: 0

ce68830

Getting this error changecalc/changecalc > changed-packages.txt || echo "./..." > changed-packages.txt fatal: bad revision 'main' failed to execute [git diff main --name-only -- .]: exit status 128 https://stackoverflow.com/a/76769122

denik temporarily deployed to test-trigger-is December 6, 2024 15:58 — with GitHub Actions Inactive

try github.event variables

7954f92

as recommended here https://github.com/orgs/community/discussions/25797#discussioncomment-3249294

denik temporarily deployed to test-trigger-is December 6, 2024 16:01 — with GitHub Actions Inactive

use refs/remotes/origin/main

2e8cb49

recommended here actions/checkout#118

denik temporarily deployed to test-trigger-is December 6, 2024 16:07 — with GitHub Actions Inactive

Revert "try github.event variables"

9e03f24

This reverts commit 7954f92.

denik temporarily deployed to test-trigger-is December 6, 2024 16:08 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is December 6, 2024 16:09 — with GitHub Actions Inactive

comment out go.mod / go.sum exception

a1dad68

denik temporarily deployed to test-trigger-is December 6, 2024 16:11 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is December 6, 2024 16:22 — with GitHub Actions Inactive

separate stages on github to view timings

bc6fa65

denik force-pushed the denis.bilenko/changecalc branch from 64b0958 to bc6fa65 Compare December 6, 2024 16:23

denik temporarily deployed to test-trigger-is December 6, 2024 16:23 — with GitHub Actions Inactive

separate stages

41cc96c

denik temporarily deployed to test-trigger-is December 6, 2024 16:27 — with GitHub Actions Inactive

shreyas-goenka reviewed Dec 9, 2024

View reviewed changes

pietern reviewed Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add change calculation tool #1975

Add change calculation tool #1975

denik commented Dec 6, 2024

denik commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024

eng-dev-ecosystem-bot commented Dec 6, 2024

shreyas-goenka left a comment

shreyas-goenka Dec 9, 2024

shreyas-goenka Dec 9, 2024

pietern Dec 9, 2024

shreyas-goenka Dec 9, 2024

shreyas-goenka Dec 9, 2024

denik Dec 9, 2024

shreyas-goenka Dec 9, 2024

pietern Dec 9, 2024

shreyas-goenka Dec 9, 2024

denik Dec 9, 2024

pietern left a comment

pietern Dec 9, 2024

pietern Dec 9, 2024

pietern Dec 9, 2024

pietern Dec 9, 2024

pietern Dec 9, 2024

Add change calculation tool #1975

Are you sure you want to change the base?

Add change calculation tool #1975

Conversation

denik commented Dec 6, 2024

Changes

Tests

denik commented Dec 6, 2024 • edited Loading

github-actions bot commented Dec 6, 2024

eng-dev-ecosystem-bot commented Dec 6, 2024

shreyas-goenka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denik commented Dec 6, 2024 •

edited

Loading