test: add validation tests for esbuild and eslint config files#19866
test: add validation tests for esbuild and eslint config files#19866mrpmohiburrahman wants to merge 5 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @mrpmohiburrahman, 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 reliability by introducing automated validation for its critical build and linting configurations. By adding dedicated unit tests for both esbuild and ESLint setups, it ensures that future changes or updates to these configurations do not inadvertently introduce regressions that could lead to broken CLI bundles, security vulnerabilities, or non-compliant code. The refactoring of the esbuild configuration was a necessary step to enable this testing, improving the overall structure and testability of the build process. Highlights
Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
There was a problem hiding this comment.
Code Review
The pull request introduces unit tests for esbuild.config.js and eslint.config.js, which is a significant improvement for maintaining the quality and stability of the project's build and linting configurations. The changes involve refactoring the esbuild configuration into a helper file (esbuild.helpers.js) to facilitate testing, and then adding comprehensive test suites for both esbuild and eslint configurations. This ensures critical aspects like native module exclusion, WASM plugin resolution, node:os import restrictions, license header enforcement, and self-import prevention are validated automatically. The new tests cover important security and build integrity aspects, which is a high-value addition to the codebase. Feedback includes a necessary copyright year update and a critical issue regarding asynchronous operations in an esbuild banner that could lead to build failures.
|
Let me know what I should do, so that this pr get accepted. |
|
Orb Code Review (powered by GLM-4.7 on Orb Cloud) SummaryThis PR adds unit tests for ArchitectureStated Goal (test file additions):
These are excellent additions - build and lint configurations had no automated validation before, which is a significant gap in test coverage. Bundled Cleanup (massive scope, unstated in PR description):The PR also removes the same subsystems as PR #20261:
IssuesPR scope — critical — Mismatch between description and actual changes The PR description states:
However, the PR includes:
Impact:
Recommendation: Split into two separate PRs:
scripts/tests/esbuild.config.test.ts — good — Comprehensive test coverage The esbuild configuration tests are well-structured and comprehensive: it('excludes node-pty to prevent bundling native addons', () => {
expect(external).toContain('node-pty');
expect(external).toContain('@lydell/node-pty');
});This correctly validates that native modules are excluded from bundling, which is critical for preventing broken bundles. it('targets node platform', () => {
expect(baseConfig.platform).toBe('node');
});Validates the correct build target, preventing platform-specific issues. it('includes CJS compatibility shim in banner', () => {
expect(cliConfig).toHaveProperty(
['banner', 'js'],
`import { createRequire } from 'module'; ...`,
);
});Ensures CJS compatibility shims are present, which is essential for backward compatibility. scripts/tests/eslint-config.test.ts — good — Validates security-critical config While I couldn't see the full content (similar to the esbuild test), adding tests for ESLint configuration is critical because:
Note: This review assumes the ESLint test follows similar patterns to the esbuild test (validating critical properties, plugins, rules, etc.). Cleanup scope — critical — Same issues as PR #20261 The cleanup bundled with this PR is nearly identical to PR #20261 and has the same issues:
Testing — minor — Tests for cleanup changes If the cleanup is intentional, the PR should include:
it('should not expose ACP commands', () => {
expect(commandRegistry.getCommand('acp')).toBeUndefined();
});
Duplicate work — warning — Overlaps with PR #20261 PRs #19866 (this one) and #20261 include nearly identical cleanup:
Questions:
PR timing — warning — Older PR not yet merged PR #19866 was created on 2026-02-22, while PR #20261 was created on 2026-02-25. Both include nearly identical cleanup. If #19866 is meant to be the primary cleanup PR, #20261 may have unnecessary duplication. If #20261 is the intended cleanup, #19866 may need to be rebased or closed. Cross-file Impact
AssessmentThe core stated goal (adding config validation tests) is excellent and addresses a significant gap in test coverage. The tests are well-structured and will prevent regressions in critical build and lint configurations. However, like PR #20261, the PR is severely scoped incorrectly by bundling this focused improvement with massive, unrelated cleanup (1,867 files changed, 1M+ deletions). Required changes:
Once split into appropriate PRs:
This approach allows for:
Note to maintainers: Given the nearly identical cleanup in PRs #19866 and #20261, please clarify which PR is the intended one and resolve the duplication before merging. |
|
The PR is solid and only needs minor edits. However, the PR cannot be merged until the branch is correctly synchronized with main to remove the unintentional deletions of unrelated subsystems. Recommended Improvement: Consistency in esbuild Banners
Next Steps:
|
Summary
Add unit tests for
esbuild.config.jsandeslint.config.jsto catchregressions in critical build and lint configurations that could lead to
broken CLI bundles or security rule violations.
Details
The build and lint configuration files had no automated validation. This PR
introduces two test suites:
esbuild.config.test.tsvalidates:node-pty,keytar) remain excluded from bundling(these contain platform-specific compiled code that would crash if bundled)
tiktoken)and relative file paths (e.g.,
./local.wasm) correctlyformat, externals) to stay consistent
eslint-config.test.tsvalidates:node:osrestrictions are enforced (e.g.,homedir,tmpdirmust usecore helpers, not
node:osdirectly, to keep sandboxing intact)and
paths.ts, which legitimately need directnode:osaccessthe Apache-2.0 header)
@google/gemini-cli-corecannot import from itself, which would causecircular dependency issues)
import/enforce-node-protocol-usagerule is active (requires usingnode:fsinstead offsfor built-in modules)no-debugger,no-console) are enabledTo make the esbuild config testable, shared logic was extracted into
esbuild.helpers.js.Note
The current test suite covers the most critical ESLint rules (security
restrictions, license headers, self-import prevention, node protocol
enforcement, and production-only safety rules). Additional rules from
eslint.config.js(e.g., TypeScript-specific overrides, importrestrictions between packages) can be added in follow-up PRs if desired.
Feedback on which rules to prioritize is welcome.
Related Issues
Fixes #16114
How to Validate
node-ptyfrom externals inesbuild.helpers.js) and confirm the tests catch the regression.Pre-Merge Checklist