googleapis / googleapis/google-cloud-node

bug(gapic-node-processing): setOnlyDefaultSystemTests incorrectly matches substring on absolute path

Open Beginner friendly
#9,342 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
3.2k
Forks
712
Avg merge
2d 3h
Merged PRs (30d)
99

Description

### Description
In `gapic-node-processing/src/combine-libraries.ts`, `setOnlyDefaultSystemTests()` filters out sample test fixtures from non-default versions when combining multi-version libraries (e.g., `google-iam` with `v2` and `v3`).

It currently uses `.includes(defaultVersion)` on the full absolute file path:
```typescript
function setOnlyDefaultSystemTests(defaultVersion: string, filePaths: FilePaths[]) {
const systemTestRegex = new RegExp('system-test/fixtures/sample/src');
for (let i = filePaths.length - 1; i >= 0; i--) {
const filePathObj = filePaths[i];
const normalizedPath = filePathObj.filePath.replace(/\\/g, '/');
if (systemTestRegex.test(normalizedPath) &&
!normalizedPath.includes(defaultVersion)) {
filePaths.splice(i, 1);
}
}
}
```

### Bug Behavior
If the workspace or temporary directory path where Librarian/generator runs happens to contain the substring `defaultVersion` (for example, `/tmp/upgrade-nodejs-v2Mp8W` containing `"v2"`), `normalizedPath.includes("v2")` evaluates to `true` for all API versions (including `v3`).

As a result:
1. `!normalizedPath.includes(defaultVersion)` evaluates to `false`.
2. `v3` fixture files are not filtered out.
3. `v3` sample fixtures overwrite `v2` sample fixtures, generating unintended client diffs (e.g., `packages/google-iam/system-test/fixtures/sample/src/index.ts` exporting `AccessPoliciesClient` instead of `PoliciesClient`).

### Reference PR & CI Breakage
This issue was observed during Librarian version upgrade in:
- **Pull Request:** https://github.com/googleapis/google-cloud-node/pull/9340
- **Affected Commit:** `559da2d4` (where `mktemp -d` generated `/tmp/upgrade-nodejs-v2Mp8W`)
- **CI Failure:** https://github.com/googleapis/google-cloud-node/actions/runs/34998266744/job/104479939975?pr=9340 (clean CI runner without `v2` in path did not generate the `v3` diffs, causing a `git diff` discrepancy).

### Suggested Fix
Match the version directory boundary specifically rather than performing an unconstrained substring check across the absolute path:
```typescript
const versionDirRegex = new RegExp(`(^|/)${defaultVersion}(/|$)`);
if (systemTestRegex.test(normalizedPath) && !versionDirRegex.test(normalizedPath)) {
filePaths.splice(i, 1);
}
```

Contributor guide

Open the contributing guide

Research direction

Start in gapic-node-processing/src/combine-libraries.ts and inspect setOnlyDefaultSystemTests(), focusing on how normalized absolute paths are matched. Reproduce or review the v2/v3 fixture case described in the issue, then verify that version filtering is unaffected by unrelated directory names such as /tmp/upgrade-nodejs-v2Mp8W and that generated fixture output no longer produces unintended diffs.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
build-system, tooling
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.