Repository navigation
pick the version out of the versioned library's directory - #9516
rootkiller6788 wants to merge 2 commits into
Conversation
setOnlyDefaultSystemTests decides which version's sample fixtures make it into a combined library, so it is worth pinning down. Three of these fail on the current implementation.
The version of a pre-combined library lives inside the directory name, like speech-v1p1beta1-nodejs, so checking whether it appears anywhere in the file path answered the wrong question. Anything above the library directory that happened to mention the version - the temp dir Librarian runs in, or the v1 inside v1p1beta1 - made every version look like the default one. So only look at that directory now.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors setOnlyDefaultSystemTests to extract and match the versioned library directory name directly, preventing false positives from paths that merely contain the version string, and adds comprehensive unit tests. Feedback on these changes suggests adding safety guards in getVersionedLibraryDir to prevent potential out-of-bounds array access when system-test is at the beginning of the path, and ensuring files are not incorrectly deleted if versionedDir cannot be resolved.
| function getVersionedLibraryDir(normalizedPath: string): string { | ||
| const parts = normalizedPath.split('/'); | ||
| const systemTestIndex = parts.indexOf('system-test'); | ||
| if (systemTestIndex < 0) return ''; | ||
| let index = systemTestIndex - 1; | ||
| if (parts[index] === 'esm') index--; | ||
| return parts[index] ?? ''; | ||
| } |
There was a problem hiding this comment.
To prevent potential out-of-bounds array access and handle cases where system-test is at the beginning of the path (index 0), we can add a guard for systemTestIndex <= 0 and ensure index is non-negative before accessing parts[index].
| function getVersionedLibraryDir(normalizedPath: string): string { | |
| const parts = normalizedPath.split('/'); | |
| const systemTestIndex = parts.indexOf('system-test'); | |
| if (systemTestIndex < 0) return ''; | |
| let index = systemTestIndex - 1; | |
| if (parts[index] === 'esm') index--; | |
| return parts[index] ?? ''; | |
| } | |
| function getVersionedLibraryDir(normalizedPath: string): string { | |
| const parts = normalizedPath.split('/'); | |
| const systemTestIndex = parts.indexOf('system-test'); | |
| if (systemTestIndex <= 0) return ''; | |
| let index = systemTestIndex - 1; | |
| if (parts[index] === 'esm') index--; | |
| return index >= 0 ? parts[index] : ''; | |
| } |
| const versionedDir = getVersionedLibraryDir(normalizedPath); | ||
| if (!versionedDir.split('-').includes(defaultVersion)) { | ||
| filePaths.splice(i, 1); | ||
| } |
There was a problem hiding this comment.
If versionedDir is empty (e.g., because system-test is not a standalone segment or cannot be resolved), we should skip the file instead of deleting it. Currently, an empty versionedDir results in [''].includes(defaultVersion) which is false, causing the file to be incorrectly deleted.
const versionedDir = getVersionedLibraryDir(normalizedPath);
if (!versionedDir) continue;
if (!versionedDir.split('-').includes(defaultVersion)) {
filePaths.splice(i, 1);
}
This branch has not been deployed
1 participant
Fixes #9342.
The version of a pre-combined library isn't a path segment of its own, it's part of the versioned library's directory name (speech-v1p1beta1-nodejs, analytics-data-v1beta-nodejs). setOnlyDefaultSystemTests was checking whether defaultVersion showed up anywhere in the absolute file path, which gets it wrong in two ways:
So it now reads the version off the versioned library's directory - the one above system-test, stepping over the esm folder for ESM libraries - and compares it as a whole against defaultVersion.
One thing worth calling out: the fix suggested in the issue (matching the version as a path segment) doesn't work. I tried it, and it drops the default version's own fixtures too, because no path segment ever equals v1beta. It would also fail the existing 'should only have default system tests' test.
Tests: 3 of the 5 new cases fail on main and pass after the change, full suite is 53 passing. I also ran generateFinalDirectoryPath against the real google-analytics-data fixture copied into a temp dir with v1beta in its name - before this the surviving sample fixture was v1alpha, now it's v1beta.