Conversation
|
![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 the CI test logging mechanism by introducing structured JSON manifests for test suites and writing logs directly to files under a .ci-logs directory. While these changes improve log management, two key issues were identified in the review: first, using synchronous file operations inside asynchronous stream handlers in run_tests_in_ci.js blocks the event loop and can cause crashes due to race conditions; second, an O(N^2) lookup complexity in print_test_logs.js should be optimized to O(N) using a Set to track already recorded log files.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the CI test logging mechanism to stream test output directly to log files and write JSON manifests for each test run, updating the log printing script to parse these manifests. Feedback on these changes includes a request to remove a deliberate crash test added to logger.test.ts for CI verification before merging, and a recommendation to wrap the process spawning and stream piping in run_tests_in_ci.js within a try-catch block to handle potential synchronous failures or undefined streams robustly.
| it('deliberate crash for CI verification', () => { | ||
| console.log('MARKER: executing deliberate crash in logger.test.ts'); | ||
| if (typeof process !== 'undefined' && process.kill) { | ||
| process.kill(process.pid, 'SIGKILL'); | ||
| } else { | ||
| throw new Error('DELIBERATE_CI_TEST_CRASH'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This test deliberately crashes the process or throws an error to verify the CI logging workflow. While useful for testing the CI pipeline changes, keeping this in the codebase will cause the test suite to fail consistently for everyone. Please remove this test before merging.
| const testProcess = spawn('yarn', ['--cwd', dir, scriptName]); | ||
|
|
||
| testProcess.childProcess.stdout.pipe(logStream, { end: false }); | ||
| testProcess.childProcess.stderr.pipe(logStream, { end: false }); | ||
|
|
||
| async function recordManifest(status, exitCode) { | ||
| try { | ||
| await new Promise(resolve => logStream.end(resolve)); | ||
| } catch (e) {} | ||
| try { | ||
| const manifestPath = path.join( | ||
| manifestsDir, | ||
| `${safeName}-${safeScript}.json` | ||
| ); | ||
| fs.writeFileSync( | ||
| manifestPath, | ||
| JSON.stringify({ | ||
| packageName: name, | ||
| scriptName, | ||
| status, | ||
| exitCode, | ||
| logFile | ||
| }) | ||
| ); | ||
| } catch (e) {} | ||
| } | ||
|
|
||
| try { | ||
| await testProcess; | ||
| await recordManifest('Success', 0); | ||
| console.log('Success: ' + name); |
There was a problem hiding this comment.
If spawn fails synchronously, or if testProcess.childProcess or its stdout/stderr streams are undefined/null, the script will throw an unhandled exception outside of the try/catch block. This will crash the script abruptly without writing a failure manifest.
To make this robust, we should define recordManifest first, and then wrap both the spawn call and the stream piping inside the try/catch block, along with defensive checks for the child process streams.
async function recordManifest(status, exitCode) {
try {
await new Promise(resolve => logStream.end(resolve));
} catch (e) {}
try {
const manifestPath = path.join(
manifestsDir,
`${safeName}-${safeScript}.json`
);
fs.writeFileSync(
manifestPath,
JSON.stringify({
packageName: name,
scriptName,
status,
exitCode,
logFile
})
);
} catch (e) {}
}
try {
const testProcess = spawn('yarn', ['--cwd', dir, scriptName]);
if (testProcess.childProcess) {
if (testProcess.childProcess.stdout) {
testProcess.childProcess.stdout.pipe(logStream, { end: false });
}
if (testProcess.childProcess.stderr) {
testProcess.childProcess.stderr.pipe(logStream, { end: false });
}
}
await testProcess;
await recordManifest('Success', 0);
console.log('Success: ' + name);
MarkDuckworth
left a comment
MarkDuckworth
left a comment
There was a problem hiding this comment.
Looks like this is effective. I'm in support of making the chage.
| const testProcess = spawn('yarn', ['--cwd', dir, scriptName]); | ||
|
|
||
| testProcess.childProcess.stdout.pipe(logStream, { end: false }); | ||
| testProcess.childProcess.stderr.pipe(logStream, { end: false }); |
There was a problem hiding this comment.
duplicate lines expected?
| env: | ||
| FIREBASE_TOKEN: ${{ secrets.FIREBASE_CLI_TOKEN }} | ||
| - name: Print Test Failure Logs | ||
| if: always() |
There was a problem hiding this comment.
is there value in printing the logs if the previous step failed?
2 participants
No description provided.