build(client): scope test jobs to changed packages#27280
Draft
tylerbutler wants to merge 25 commits into
Draft
Conversation
Adds an opt-in `detect_changes` job to the client build pipeline that
computes the merge-base vs the PR's target branch and emits a pnpm
`--filter "...[<sha>]"` expression as an output variable. Downstream
test jobs consume it via `npm_config_filter`, so pnpm natively scopes
recursive runs to packages changed in the PR (plus their transitive
dependents) — no wrapper scripts, no package.json changes.
Activates when one of:
* `enableChangedPackageTestScoping` pipeline parameter is true, OR
* the PR source branch name contains `test/filtered-ci/` — a
branch-name convention that lets contributors exercise the feature
from an auto-triggered PR build without flipping parameters.
When the detect job is skipped (non-PR builds, or PR builds that
haven't opted in) its output variables resolve to empty strings, which
downstream jobs interpret as "no filter — run every package" — so
pipeline behavior is effectively byte-identical to today for non-opt-in
runs.
The merge-base SHA is passed to pnpm's selector instead of a branch ref
because pnpm's `[ref]` uses a two-dot diff (pnpm/pnpm#9907), which
would pick up unrelated commits from the target branch as "changed".
🤖 Generated with [Nori](https://noriagentic.com)
Co-Authored-By: Nori <contact@tilework.tech>
Addresses review feedback on the scoping PR: - Skip Coverage_tests and Test_* jobs entirely (not just their test steps) when detect_changes reports shouldRunTests=false. Empty (detect_changes skipped) still runs, preserving the historical non-opt-in behavior. Saves agent allocation + checkout + install for no-package-change PRs. - Remove the now-redundant step-level Start Test condition and orphaned `shouldRunTests` job variable; `scopedPnpmFilter` is still exposed as a job variable because test-task steps consume it. - Shallow-clone the detect_changes checkout (fetchDepth: 200) with a runtime unshallow fallback guarded by .git/shallow presence, so full clones don't error on --unshallow. - Cross-reference `pr: paths: include:` in build-client.yml from FULL_RUN_PATTERNS to flag the convention-based overlap. - Condense duplicated "historical behavior" narration across the diff. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
PR microsoft#27134 revealed that `succeeded()` on `Coverage_tests` and `Test_*` false-skips the job when `detect_changes` is Skipped, contrary to the documented "skipped deps count as successful" behavior. Check `dependencies.build.result` directly so the test jobs run for non-opt-in PRs (where `detect_changes` is intentionally skipped) while still respecting a 'false' output signal when scoping is active. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
The parameter only takes effect on manually-queued builds, where an operator can tick it in the ADO UI. Auto-triggered PR builds always run with the `false` default, so the only practical opt-in is the 'test/filtered-ci/' branch-name substring. Removing the parameter eliminates a lever that can't be pulled from a developer's normal workflow and simplifies the detect_changes activation condition. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Addresses PR review feedback on microsoft#27133: - git diff failures after merge-base resolution now emit a pipeline warning and call emit_and_exit (full run) instead of swallowing the error into an empty CHANGED_FILES, which would bypass full-run patterns and suppress all tests. - git rev-parse --git-dir is captured into GIT_DIR with a 2>/dev/null || true fallback and the shallow-file check gates on [ -n "${GIT_DIR}" ], so a rev-parse failure skips the unshallow branch instead of aborting under set -eu. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Addresses PR review H1: wrapping the env var in `${{ if ne(parameters.pnpmFilter, '') }}`
keeps the contract explicit — we only ask pnpm to filter when a filter is
actually set. Empirically pnpm 10 treats an empty value the same as unset, but
the conditional avoids relying on that unverified behavior.
🤖 Generated with [Nori](https://noriagentic.com)
Co-Authored-By: Nori <contact@tilework.tech>
Addresses PR review M1 without introducing a bash unit test framework. The HAS_PACKAGE_CHANGE=false path is the most aggressive skip in the template — it causes every test job to be bypassed. Previously an accidental misclassification (bug in the directory walk, unexpected file layout) would be invisible in the pipeline summary: the skip was logged with a plain echo and the specific files considered were not dumped. Now that branch emits an ADO warning (`##vso[task.logissue type=warning]`) and prints the full list of files that went into the decision, so the suppression is auditable from the pipeline run without a re-run. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Responds to the review threads on microsoft#27133: - Extract the inline bash from include-detect-changed-packages.yml into scripts/detect-changed-packages.ts — runnable/debuggable locally and covered by unit tests (jason-ha, alexvy86). - Add node:test unit tests under scripts/test/ and wire them into the build job (runs early, fails fast, no new test framework). - Add .pnpmfile.cjs, .npmrc, .nvmrc to FULL_RUN_PATTERNS — root-level install/runtime config that affects every package (Copilot, alexvy86). - Fix deleted-package detection: union the merge-base tree's package.json list with the working tree so a package removed on this branch still maps to a known package dir (Copilot). - Unfilter the non-recursive `pnpm puppeteer` call in test:jest* via `cross-env npm_config_filter=`; confirmed empirically that npm_config_filter propagates to every pnpm invocation, not just recursive ones (Copilot). 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Replaces the ad-hoc `npx --yes tsx@<version>` invocations with a lockfile- pinned `tsx` devDependency called via `pnpm exec tsx`. Version is managed in pnpm-lock.yaml rather than hardcoded in pipeline YAML, so bumps go through the same flow as every other dep. detect_changes now installs workspace-root deps before invoking tsx — mirrors repo-policy-check.yml's fast-path (full workspace install is still avoided). The build job's "Scripts unit tests" step moved to run right after include-install.yml so tsx is already resolvable when it runs. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
- _git() now logs a pipeline warning (command + stderr) on failure instead of swallowing the error silently. - Rename find_changed_packages → any_changed_file_in_packages so the name reflects the bool return. - Drop the pattern-list-audit test and collapse the three redundant any_changed_file_in_packages happy-path tests; the deleted/new package cases are already covered by the build_package_dir_set union test. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
…comments - Replace os.walk with git ls-files in _current_packages (symmetric with the historical helper, respects .gitignore). - Diff merge_base..HEAD instead of merge_base alone, so working-tree mutations from any pre-step can't leak into the changed-files list. - Switch --unshallow to --deepen 1000 to avoid pulling full history when the merge-base is just past the shallow boundary. - Use rev-parse --is-shallow-repository instead of probing .git/shallow. - Extract _resolve_merge_base and _fallback_full_run helpers. - Strip meta-narrating comments and trim duplicated YAML docstrings. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
- Drop dead branches relying on posix dirname returning "" (it doesn't). - Inline buildPackageDirSet's record helper now that the dot fallback is unnecessary. - Trim merge-base output once at the call site. - Collapse repetitive checkFullRunPatterns "matches X" tests into a table-driven loop and split the combined root/nested package.json case.
Annotate every function (exports and internals) with @param/@returns and a short description focused on the why — return-on-error semantics, the endpoint union in buildPackageDirSet, the root pseudo-dir exclusion in anyChangedFileInPackages, and the shallow-clone deepen retry in resolveMergeBase. Also type FULL_RUN_PATTERNS as readonly RegExp[].
Contributor
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (696 lines, 17 files), I've queued these reviewers:
How this works
|
tylerbutler
changed the title
Reorganizes the check changedPackages command and supporting library code so the command file contains only command logic. - Moves normalizeTargetBranch from the command into library/branches.ts. - Replaces the local resolveMergeBase helper with the shallow-aware getMergeBaseRemote from @fluid-tools/build-infrastructure. - Replaces the local package-dir helpers (buildPackageDirSet, anyChangedFileInPackages, packageJsonFilesFromGitOutput) with the new getPackageDirsAtRef and isFileInPackageDir primitives from @fluid-tools/build-infrastructure. - Switches the command's ##vso[...] emitters to the shared formatSetVariable and formatLogIssue helpers in library/azureDevops/pipelineCommands.ts. The command now exports only the public ChangedPackagesResult type and the oclif class. The fullRunPatterns array and findFullRunPatternMatch helper remain command-local since they encode CI-specific scoping policy. Also moves the targetBranch flag's TARGET_BRANCH default into the flag definition via the env property. This change depends on PR microsoft#27283 (build-infra helpers) and PR microsoft#27284 (ADO pipeline command helpers); rebase onto main after both merge to drop the duplicated content from this branch's diff.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
How contribute to this repo.
Guidelines for Pull Requests.
Description
Adds
flub check changedPackagesto compute changed-package test scoping from the PR merge base and emit Azure DevOps variables for downstream pipeline jobs. Moving this logic intoflubmakes the behavior typed, reusable, and easier to maintain than inline pipeline script logic.The client package pipeline now runs a
detect_changesjob before coverage and task test jobs. That job reports whether tests should run at all and, when applicable, provides a pnpm filter rooted at the merge base so downstream jobs can run only packages affected by the PR plus their dependents.This behavior is intentionally gated during rollout:
detect_changesonly runs for PR builds whose source branch name containstest/filtered-ci/. PRs from other branch names keep the existing full-test behavior because the detection job is skipped and no pnpm filter is applied.Detection remains conservative. Repository-wide changes, missing branch/remote data, or unexpected detection failures fall back to a full test run; jobs are skipped only when detection explicitly reports that no workspace package files were affected.
This also clears inherited pnpm filters around root-level Puppeteer installs so scoped test runs do not break non-recursive pnpm commands.
Reviewer Guidance
The review process is outlined on this wiki page.