ref(selective-testing): Re-add make test-selective command#113265
ref(selective-testing): Re-add make test-selective command#113265
Conversation
| mkdir -p .cache && > .cache/selected-tests.txt | ||
| python3 .github/workflows/scripts/compute-sentry-selected-tests.py \ | ||
| --coverage-db .cache/coverage.db \ | ||
| --changed-files "$$(git diff --name-only $$(git merge-base origin/master HEAD))" \ |
There was a problem hiding this comment.
Bug: The test selection script uses split() on a space-separated string of filenames, which will fail if any filename contains a space.
Severity: LOW
Suggested Fix
To handle filenames with spaces, the script should not rely on simple splitting. The Makefile could pass filenames separated by a unique, non-filename character (like a newline), and the Python script could split on that character. Alternatively, use a more robust argument passing mechanism like xargs or pass each file as a separate argument.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: Makefile#L163
Potential issue: The `Makefile` passes a list of changed files as a single
space-separated string to the `compute-sentry-selected-tests.py` script. The script
parses this string using `args.changed_files.split()`, which splits on any whitespace.
If a developer commits a file with a space in its name, the filename will be incorrectly
split into multiple parts, causing the test selection logic to fail and potentially skip
running relevant tests. This is a bug in a developer tool.
Did we get this right? 👍 / 👎 to inform future reviews.
| mkdir -p .cache && > .cache/selected-tests.txt | ||
| python3 .github/workflows/scripts/compute-sentry-selected-tests.py \ | ||
| --coverage-db .cache/coverage.db \ | ||
| --changed-files "$$(git diff --name-only $$(git merge-base origin/master HEAD))" \ |
There was a problem hiding this comment.
Bug: The test-selective Makefile target hardcodes origin/master, which can fail for developers using a different remote name or a main default branch, causing incorrect test selection.
Severity: MEDIUM
Suggested Fix
Dynamically determine the default branch instead of hardcoding origin/master. For example, use a command like git symbolic-ref refs/remotes/origin/HEAD to find the remote's default branch. Additionally, consider adding error checking to the git commands to alert the user if the base branch cannot be determined.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: Makefile#L163
Potential issue: The `test-selective` target in the Makefile uses a hardcoded
`origin/master` reference in its `git merge-base` command. This will fail for developers
whose remote is not named `origin` or whose main branch is `main`. When the `git
merge-base` command fails, Make's `shell` function returns an empty string without
halting execution. Consequently, `git diff --name-only` is called with no arguments,
causing it to compare the working directory to the staging area instead of the intended
branch. This leads to an incorrect selection of tests being run, without any error
message to inform the developer of the underlying issue.

Allows for
make test-selectiveto run just tests against changed files locally. Downloads latest coverage data since shas won't match getsentry shas, but should be good enough.Will force a gcloud cli login to be able to fetch the coverage data. We'll wire this up to a service as a follow on/improvement soon