googleapis / googleapis/google-cloud-node
bug(gapic-node-processing): setOnlyDefaultSystemTests incorrectly matches substring on absolute path
- 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
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