feat(testing): run tests in modified packages only #4214

muncus · 2024-06-25T21:29:07Z

current testing logic also ensures modules under these packages are
also tested.

current testing logic also ensures modules *under* these packages are also tested.

Deleplace · 2024-07-01T09:44:33Z

testing/kokoro/system_tests.sh

-GO_CHANGED_MODULES=${GO_CHANGED_MODULES:-./go.mod}
-# Exclude the root module, if present, from the list of sub-modules.
-GO_CHANGED_SUBMODULES=${GO_CHANGED_MODULES#./go.mod}
+GO_CHANGED_MODULES="$(find ${GO_CHANGED_PKGS:-.} -name go.mod | xargs --no-run-if-empty dirname)"


I tried to execute the steps but encountered these problems:

GO_CHANGED_PKGS would contain a list of go files, not a list of folders

GO_CHANGED_MODULES does not contain the modified modules

Maybe dirname could help:

$ dirname vertexai/chat/chat.go vertexai/chat

Thanks for taking a look at this!

I've added dirname to this pipeline, and to make future changes easier, i've allowed us to set $GIT_CHANGES locally to enable better testability here.

So running GIT_CHANGES="functions/functionsv2/firebase/hellofirestore/go.mod compute/instances/foo.go" sh testing/kokoro/system_tests.sh will show (among other things): GO_CHANGED_PKGS='compute/instances functions/functionsv2/firebase/hellofirestore'

the script stops when it tries to read the nonexistant kokoro credentials, but this is still an improvement.

Thanks!
Now the value of GO_CHANGED_MODULES looks good, but I can't see it used anywhere in the script?

That's correct. the module-based approach was never fully implemented, so these variables are informational only.

also enables local debugging by setting $GIT_CHANGES

grayside

Overall this looks good to me. I wasn't able to get some of the specific script elements working locally on Mac but it makes sense.

grayside · 2024-07-08T23:39:57Z

testing/kokoro/system_tests.sh

 # CHANGED_DIRS is the list of significant top-level directories that changed,
 # but weren't deleted by the current PR.
 # CHANGED_DIRS will be empty when run on main.
 CHANGED_DIRS=$(echo "$SIGNIFICANT_CHANGES" | tr ' ' '\n' | grep "/" | cut -d/ -f1 | sort -u | tr '\n' ' ' | xargs --no-run-if-empty ls -d 2>/dev/null || true)
+GO_CHANGED_PKGS=$(echo "$SIGNIFICANT_CHANGES" | tr ' ' '\n' | grep "/" | tr '\n' ' ' | xargs --no-run-if-empty dirname | xargs --no-run-if-empty ls -d 2>/dev/null || true)


question: Not recognizing the logic indicating these directories are necessarily Go packages. What am I missing?

A Package is just a directory that contains go code. a Module is a grouping of packages (see https://go.dev/ref/mod#modules-overview).

We are not looking for the module that contains the modified packages, we are just testing the modified packages themselves. We believe this is safe due to the rarity of inter-package dependencies, and the use of exhaustive nightly testing.

if we do experience breakage, we can move toward a "find the containing module" approach, but that would be more complex.

Deleplace

Marc, I tested the modified changes in bash (macOS) and now I can confirm the env var values look good 👍

Before LGTM, I'd want to understand if GO_CHANGED_MODULES is supposed to be used at all?

Deleplace · 2024-07-09T08:01:11Z

testing/kokoro/system_tests.sh

@@ -44,25 +44,27 @@ export GOLANG_SAMPLES_E2E_TEST=""
 # allow files to be owned by a different user than our current uid.
 # Kokoro runs a double-nested container, and UIDs may not match.
 git config --global --add safe.directory $(pwd)
-GIT_CHANGES=$(git --no-pager diff --name-only main..HEAD)
+# Allow $GIT_CHANGES to be set in the env, enabling local testing of the change detection below.
+GIT_CHANGES=${GIT_CHANGES:-$(git --no-pager diff --name-only main..HEAD)}


The git changes between HEAD and main includes the current changes, but they also includes any very recent changes of main, unrelated to the current changes. Is this intentional?

main here is using the current status of the main branch in your checkout - which is likely missing some of the newest changes to origin/main. you can make those changes disappear by doing a git pull on your main branch, then switching back to the PR branch (and ensuring the PR branch is up to date with main).

This is not a problem in CI, where we're using a fresh clone of the repo.

We may be able to address this by using origin/main instead of main, but that's probably unnecessary for our usage, and i'd like to minimize the behavior change in this PR. If you think this is an important change, you're welcome to file an issue for it.

Deleplace · 2024-07-09T08:44:09Z

testing/kokoro/system_tests.sh

 if [[ -z $GIT_CHANGES && $KOKORO_JOB_NAME != *"system-tests"* ]]; then
  echo "No diffs detected. This is unexpected - check above for additional error messages."
  exit 2
 fi
-SIGNIFICANT_CHANGES=$(git --no-pager diff --name-only main..HEAD | grep -Ev '(\.md$|^\.github)' || true )
+SIGNIFICANT_CHANGES=$(echo $GIT_CHANGES | grep -Ev '(\.md$|^\.github)' || true )
 # CHANGED_DIRS is the list of significant top-level directories that changed,
 # but weren't deleted by the current PR.
 # CHANGED_DIRS will be empty when run on main.
 CHANGED_DIRS=$(echo "$SIGNIFICANT_CHANGES" | tr ' ' '\n' | grep "/" | cut -d/ -f1 | sort -u | tr '\n' ' ' | xargs --no-run-if-empty ls -d 2>/dev/null || true)


CHANGED_DIRS includes new dirs added in the current change, but not the new dirs very recently added in main. Is this intentional?

I don't understand how that's possible, since the git command is the same, and i was not able to reproduce this behavior.
The presence of non-PR changes is dependent on the git clone's current state- could that have changed for you between these commands?

Deleplace · 2024-07-09T09:16:21Z

testing/kokoro/system_tests.sh

-GO_CHANGED_MODULES=${GO_CHANGED_MODULES:-./go.mod}
-# Exclude the root module, if present, from the list of sub-modules.
-GO_CHANGED_SUBMODULES=${GO_CHANGED_MODULES#./go.mod}
+GO_CHANGED_MODULES="$(find ${GO_CHANGED_PKGS:-.} -name go.mod | xargs --no-run-if-empty dirname)"


Thanks!
Now the value of GO_CHANGED_MODULES looks good, but I can't see it used anywhere in the script?

muncus · 2024-07-15T20:20:58Z

@grayside, @Deleplace Ping. This PR has been awaiting re-review for a week. Could one of you take a look so we can get this speed improvement delivered?

feat(testing): run tests in modified packages only

8dace07

current testing logic also ensures modules *under* these packages are also tested.

product-auto-label bot added the samples Issues that are directly related to samples. label Jun 25, 2024

Merge branch 'main' into smoltest

a1d8039

muncus marked this pull request as ready for review June 27, 2024 17:21

muncus requested a review from a team as a code owner June 27, 2024 17:21

muncus added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 27, 2024

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 27, 2024

Deleplace requested changes Jul 1, 2024

View reviewed changes

muncus added 2 commits July 2, 2024 14:38

fix: use dirname as suggested by @Deleplace

4877a5e

also enables local debugging by setting $GIT_CHANGES

Merge branch 'main' into smoltest

0ddf087

muncus requested a review from Deleplace July 2, 2024 22:17

muncus added 2 commits July 2, 2024 16:41

Merge branch 'main' into smoltest

d2fd855

Merge branch 'main' into smoltest

a31f7f3

grayside reviewed Jul 8, 2024

View reviewed changes

Deleplace requested changes Jul 9, 2024

View reviewed changes

muncus added 2 commits July 9, 2024 09:14

Merge branch 'main' into smoltest

7ee5d15

Merge branch 'main' into smoltest

53ca722

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(testing): run tests in modified packages only #4214

feat(testing): run tests in modified packages only #4214

muncus commented Jun 25, 2024

Deleplace Jul 1, 2024

Deleplace Jul 1, 2024

muncus Jul 2, 2024

Deleplace Jul 9, 2024

muncus Jul 9, 2024

grayside left a comment

grayside Jul 8, 2024

muncus Jul 9, 2024

Deleplace left a comment

Deleplace Jul 9, 2024

muncus Jul 9, 2024 •

edited

Loading

Deleplace Jul 9, 2024

muncus Jul 9, 2024

Deleplace Jul 9, 2024

muncus commented Jul 15, 2024

feat(testing): run tests in modified packages only #4214

Are you sure you want to change the base?

feat(testing): run tests in modified packages only #4214

Conversation

muncus commented Jun 25, 2024

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

grayside left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deleplace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muncus Jul 9, 2024 • edited Loading

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

muncus commented Jul 15, 2024

muncus Jul 9, 2024 •

edited

Loading