feat(benchmarks): Unify memory tests and add CI workflow#802
Conversation
- Consolidate memory-leak-test.ts and simple-memory-test.ts into single memory-test.ts - Add flexible CLI options: --full for comprehensive analysis, --continuous for monitoring - Update package.json scripts to use simplified commands (test, test:full, test:continuous) - Add benchmark.yml GitHub Actions workflow for automated memory testing - CI runs lightweight tests on PR/push, full analysis available via manual dispatch - Memory growth threshold causes CI failure to catch regressions early
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a new GitHub Actions workflow for memory benchmarking, replaces legacy memory npm scripts with standardized test commands, refactors the memory test into a single flag-driven CLI with altered summary shape, and removes the old simple-memory-test implementation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer/CI
participant GA as GitHub Actions
participant Job as memory-test job
participant Node as Node + Benchmarks
Dev->>GA: push/PR/dispatch
GA->>Job: Start "memory-test"
Job->>Node: Checkout, setup Node (cache)
Job->>Node: npm ci (root) + build
Job->>Node: npm ci (benchmarks) + build
Job->>Node: Run node --expose-gc dist/memory-test.js [flags]
Node-->>Job: Emit JSON results (if any)
Job->>GA: Upload artifact (results)
GA-->>Dev: Workflow complete
note over Job,Node: "memory-test-full" mirrors steps with --full/--save and longer timeout
sequenceDiagram
autonumber
actor User as CLI User/CI
participant MT as memory-test.js
participant CLI as Flag Parser
participant Runner as Test Loop
participant FS as Filesystem
User->>MT: node dist/memory-test.js [--full|--continuous|--save] [iterations] [delay]
MT->>CLI: Parse flags and args
CLI-->>MT: Mode, iterations, delay, save
MT->>Runner: Initialize (thresholds, intervals)
loop Iterations (∞ if continuous, bounded in CI)
Runner->>Runner: runCli(TEST_CONFIG)
Runner->>Runner: GC/logging per mode
alt --save or --full
Runner->>FS: Append memory sample
end
end
MT->>Runner: Final analysis (heap/RSS growth)
alt Growth exceeds threshold in CI
MT->>User: Exit non-zero
else Growth within threshold
MT->>User: Exit zero
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @yamadashy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the project's memory testing capabilities by consolidating existing scripts and integrating automated memory regression detection into the CI pipeline. The core change involves unifying two separate memory test scripts into a single, highly configurable memory-test.ts file. This new script supports various modes, including a lightweight version for continuous integration, a comprehensive analysis mode, and a continuous monitoring option. Alongside this, the PR introduces a new GitHub Actions workflow that automatically runs these memory tests on every code change, ensuring that any potential memory leaks or significant memory growth are caught early in the development cycle. This improves the overall stability and performance of the codebase by proactively identifying and preventing memory-related issues.
Highlights
- Unified Memory Testing Infrastructure: The separate
memory-leak-test.tsandsimple-memory-test.tsscripts have been merged into a single, more robustmemory-test.tsfile. This new script offers flexible options via command-line flags (--full,--continuous,--save) to cater to different testing needs, from quick CI checks to comprehensive analysis. - Simplified CLI and Package Scripts: The
package.jsonscripts have been streamlined, replacing the oldmemory:check,memory:watch, andmemory:analyzewith intuitivetest,test:full, andtest:continuouscommands. This simplifies the execution of various memory test scenarios. - Automated CI Integration for Memory Regression Detection: A new
benchmark.ymlGitHub Actions workflow has been added to automate memory testing. This workflow runs on every pull request and push, executing a basic memory test. It is configured to fail the CI build if significant memory growth (exceeding a defined threshold) is detected, preventing memory regressions from being merged.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates memory testing infrastructure into a unified solution that supports both lightweight CI testing and comprehensive analysis. The changes remove redundant test files and create a flexible, configurable memory testing tool with automated CI integration.
- Unified
simple-memory-test.tsandmemory-leak-test.tsinto singlememory-test.tswith configurable modes - Added CLI flags (
--full,--continuous,--save) for different testing scenarios - Integrated automated memory testing into CI with failure detection for memory growth > 100%
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
benchmarks/memory/src/simple-memory-test.ts |
Removed - functionality merged into unified memory test |
benchmarks/memory/src/memory-test.ts |
Enhanced with configurable modes, CLI flags, and CI-friendly defaults |
benchmarks/memory/package.json |
Updated scripts to use unified test commands |
.github/workflows/benchmark.yml |
Added new CI workflow for automated memory testing |
Deploying repomix with
|
| Latest commit: |
4f9f5ae
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://18706689.repomix.pages.dev |
| Branch Preview URL: | https://chore-memory-check-ci.repomix.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #802 +/- ##
=======================================
Coverage 87.41% 87.41%
=======================================
Files 113 113
Lines 6493 6493
Branches 1331 1331
=======================================
Hits 5676 5676
Misses 817 817 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request effectively unifies the memory testing scripts into a single, more flexible memory-test.ts with clear command-line options. The addition of a CI workflow with memory regression detection is a great improvement for repository health. The code is well-structured and the changes significantly improve the benchmarking infrastructure.
I have a few suggestions to improve the new test script further: one to prevent a potential runtime error, another to align its documentation with the implementation, and a third to make the argument parsing more robust.
Remove package-lock.json from .gitignore to ensure CI can run npm ci properly with locked dependencies for reliable builds.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
benchmarks/memory/package.json (1)
12-14: Avoid "node --run" unless you require Node 22+; use npm run for broader compatibility."node --run" is a relatively new Node feature and won’t work on Node 18/20. Your engines field allows >=18, so these scripts can break in local/dev environments. Either bump engines to Node 22+ or switch to npm run.
Apply one of the following diffs:
Option A — keep engines as is, replace "node --run" with npm:
- "test": "node --run build:all && node --expose-gc dist/memory-test.js", - "test:full": "node --run build:all && node --expose-gc dist/memory-test.js --full", - "test:continuous": "node --run build:all && node --expose-gc dist/memory-test.js --continuous" + "test": "npm run build:all && node --expose-gc dist/memory-test.js", + "test:full": "npm run build:all && node --expose-gc dist/memory-test.js --full", + "test:continuous": "npm run build:all && node --expose-gc dist/memory-test.js --continuous"Also update the internal build helpers:
- "build:repomix": "cd ../.. && node --run build", - "build:all": "node --run build:repomix && node --run build", + "build:repomix": "cd ../.. && npm run build", + "build:all": "npm run build:repomix && npm run build",Option B — require Node 22+:
- "engines": { - "node": ">=18.0.0" - }, + "engines": { + "node": ">=22.6.0" + },Also applies to: 23-25
benchmarks/memory/src/memory-test.ts (6)
4-8: Comment mentions a non-existent “--configs” flag; remove or implement to avoid drift.The header advertises “Multiple configurations support (--configs flag)” but the CLI doesn’t parse such a flag and the test runs a single TEST_CONFIG. This will confuse maintainers.
Apply this diff to correct the header comment now (or follow up with a separate feature to add the flag):
- * - Multiple configurations support (--configs flag) + * - Single configuration (can be extended to multiple configs in future)
52-74: Help text shows .ts entry; runtime uses built JS / shebang.Examples and usage should reflect the built artifact (dist/memory-test.js) or the shebang form to prevent confusion.
-Usage: node memory-test.ts [iterations] [delay] [options] +Usage: node dist/memory-test.js [iterations] [delay] [options] @@ - node memory-test.ts # Quick CI test (50 iterations) - node memory-test.ts --full # Comprehensive test (200 iterations) - node memory-test.ts 100 200 # 100 iterations, 200ms delay - node memory-test.ts --continuous # Run until Ctrl+C + node dist/memory-test.js # Quick CI test (50 iterations) + node dist/memory-test.js --full # Comprehensive test (200 iterations) + node dist/memory-test.js 100 200 # 100 iterations, 200ms delay + node dist/memory-test.js --continuous # Run until Ctrl+CAlternatively, if you rely on the shebang: “./dist/memory-test.js …”.
28-32: Skip iterations validation when running in --continuous mode.In continuous mode iterations are ignored, but the validator still enforces a positive count and can reject “0” even though it won’t be used.
-if (Number.isNaN(iterations) || iterations <= 0) { +if (!flags.continuous && (Number.isNaN(iterations) || iterations <= 0)) { console.error('❌ Invalid iterations count. Must be a positive number.'); process.exit(1); }Also applies to: 295-304
124-132: cleanupFiles swallows unexpected fs errors. Log non-ENOENT errors even if they don’t expose a “code” property.Right now only errors with code !== ENOENT are logged; other error shapes get silently ignored.
- } catch (error) { - if (error instanceof Error && 'code' in error && error.code !== 'ENOENT') { - console.warn(`Failed to cleanup ${TEST_CONFIG.options.output}:`, error.message); - } - } + } catch (error) { + const err = error as NodeJS.ErrnoException; + if (!err || (err.code && err.code !== 'ENOENT') || (!('code' in (err as object)))) { + console.warn(`Failed to cleanup ${TEST_CONFIG.options.output}:`, err?.message ?? String(err)); + } + }
248-255: Guard against divide-by-zero in growth calculations to avoid Infinity/NaN.While unlikely, defensive checks make the final analysis safer.
- const heapGrowth = (((finalUsage.heapUsed - initialUsage.heapUsed) / initialUsage.heapUsed) * 100); - const rssGrowth = (((finalUsage.rss - initialUsage.rss) / initialUsage.rss) * 100); + const safeDiv = (num: number, den: number) => den > 0 ? (num / den) : 0; + const heapGrowth = safeDiv(finalUsage.heapUsed - initialUsage.heapUsed, initialUsage.heapUsed) * 100; + const rssGrowth = safeDiv(finalUsage.rss - initialUsage.rss, initialUsage.rss) * 100;
282-287: Add unhandledRejection and SIGTERM handlers for graceful cleanup.This improves resilience in CI cancellations and promise rejection paths.
process.on('uncaughtException', async (error) => { console.error('\n❌ Uncaught exception:', error); await saveMemoryHistory(); await cleanupFiles(); process.exit(1); }); + +process.on('unhandledRejection', async (reason) => { + console.error('\n❌ Unhandled rejection:', reason); + await saveMemoryHistory(); + await cleanupFiles(); + process.exit(1); +}); + +process.on('SIGTERM', async () => { + console.log('\n\n⚠️ Received SIGTERM'); + await saveMemoryHistory(); + await cleanupFiles(); + process.exit(0); +});.github/workflows/benchmark.yml (3)
65-71: Artifact upload may fail when no results are produced; set if-no-files-found to ignore.Basic runs don’t save results by default, so the artifact step can be noisy or fail depending on action defaults. Make it explicit.
- name: Upload memory test results uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # ratchet:actions/upload-artifact@v4 if: always() with: name: memory-test-results-${{ github.run_id }} path: benchmarks/memory/memory-test-results-*.json + if-no-files-found: ignore retention-days: 30
1-123: Trim trailing spaces and add a final newline to satisfy YAML linters.YAMLlint flagged trailing spaces at Lines 36, 41, 46, 50, 54, 63, 91, 96, 101, 105, 109, 115 and a missing newline at EOF. Please remove the trailing spaces on those lines and ensure the file ends with a newline. This keeps CI tidy.
55-63: Consider invoking npm scripts for consistency with package.json.Not required, but using package scripts keeps flags and env aligned with local runs and reduces drift between CI and dev usage.
Possible change:
- node --expose-gc dist/memory-test.js $ITERATIONS $DELAY + npm test --silent -- $ITERATIONS $DELAYand for full:
- run: node --expose-gc dist/memory-test.js --full --save + run: npm run -s test:fullAlso applies to: 110-115
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/benchmark.yml(1 hunks)benchmarks/memory/package.json(1 hunks)benchmarks/memory/src/memory-test.ts(9 hunks)benchmarks/memory/src/simple-memory-test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- benchmarks/memory/src/simple-memory-test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
benchmarks/memory/src/memory-test.ts (2)
benchmarks/memory/src/types.ts (2)
TestConfig(21-33)MemoryHistory(9-19)website/server/src/utils/memory.ts (1)
getMemoryUsage(21-39)
🪛 YAMLlint (1.37.1)
.github/workflows/benchmark.yml
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 91-91: trailing spaces
(trailing-spaces)
[error] 96-96: trailing spaces
(trailing-spaces)
[error] 101-101: trailing spaces
(trailing-spaces)
[error] 105-105: trailing spaces
(trailing-spaces)
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 115-115: trailing spaces
(trailing-spaces)
[error] 123-123: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and run (windows-latest, 20.x)
- GitHub Check: Build and run with Bun (windows-latest, latest)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
benchmarks/memory/src/memory-test.ts (1)
306-315: Nice UX touches.Mode/features banner and Ctrl+C hint are clear; this helps operators quickly see what’s running.
.github/workflows/benchmark.yml (1)
19-27: Workflow structure looks solid.Good defaults, timeouts, pinned SHAs, and caching. Using defaults.run.working-directory simplifies steps and reduces duplication.
Also applies to: 73-82
- Remove misleading --configs flag reference from file comment - Add division by zero protection for memory growth calculations - Prevent potential Infinity/NaN values in test results
Remove CI-only condition - memory leak detection should fail the command regardless of environment for consistent behavior
yamadashy
force-pushed
the
chore/memory-check-ci
branch
from
a0c70ac to
4f9f5ae
Compare
This PR consolidates the memory testing infrastructure and adds automated CI testing.
Summary
memory-leak-test.tsandsimple-memory-test.tsinto a singlememory-test.tswith flexible options--full,--continuous,--saveflags for different testing scenariostest,test:full,test:continuousbenchmark.ymlworkflow for automated memory testing on PR/pushKey Features
🧪 Unified Memory Test
--full): 200 iterations with detailed analysis--continuous): Run until stopped--save): Export detailed JSON reports🚀 CI Workflow
Testing
Checklist
npm run testnpm run lintactionlint