From 17331001a0ac10212097c50797eda287296bf27a Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Fri, 25 Jul 2025 15:57:30 -0700 Subject: [PATCH] Run presubmit tests in windows as well as linux. (#4672) Co-authored-by: matt korwel --- .../actions/post-coverage-comment/action.yml | 5 +- .github/workflows/ci.yml | 18 +- packages/core/src/services/gitService.test.ts | 180 ++++++++---------- packages/core/src/tools/shell.test.ts | 8 +- 4 files changed, 96 insertions(+), 115 deletions(-) diff --git a/.github/actions/post-coverage-comment/action.yml b/.github/actions/post-coverage-comment/action.yml index 20b67019..10a4afeb 100644 --- a/.github/actions/post-coverage-comment/action.yml +++ b/.github/actions/post-coverage-comment/action.yml @@ -17,6 +17,9 @@ inputs: node_version: description: 'Node.js version for context in messages' required: true + os: + description: 'The os for context in messages' + required: true github_token: description: 'GitHub token for posting comments' required: true @@ -91,7 +94,7 @@ runs: echo "" >> "$comment_file" echo "" >> "$comment_file" - echo "_For detailed HTML reports, please see the 'coverage-reports-${{ inputs.node_version }}' artifact from the main CI run._" >> "$comment_file" + echo "_For detailed HTML reports, please see the 'coverage-reports-${{ inputs.node_version }}-${{ inputs.os }}' artifact from the main CI run._" >> "$comment_file" - name: Post Coverage Comment uses: thollander/actions-comment-pull-request@v3 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6846d4b5..e00b1f71 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,7 +44,7 @@ jobs: test: name: Test - runs-on: ubuntu-latest + runs-on: ${{ matrix.os }} needs: lint permissions: contents: read @@ -52,6 +52,7 @@ jobs: pull-requests: write strategy: matrix: + os: [ubuntu-latest, windows-latest] node-version: [20.x, 22.x, 24.x] steps: - name: Checkout repository @@ -70,7 +71,9 @@ jobs: run: npm ci # Install fresh dependencies using the downloaded package-lock.json - name: Run tests and generate reports - run: NO_COLOR=true npm run test:ci + run: npm run test:ci + env: + NO_COLOR: true - name: Publish Test Report (for non-forks) if: always() && (github.event.pull_request.head.repo.full_name == github.repository) @@ -85,14 +88,14 @@ jobs: if: always() && (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: - name: test-results-fork-${{ matrix.node-version }} + name: test-results-fork-${{ matrix.node-version }}-${{ matrix.os }} path: packages/*/junit.xml - name: Upload coverage reports uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 if: always() with: - name: coverage-reports-${{ matrix.node-version }} + name: coverage-reports-${{ matrix.node-version }}-${{ matrix.os }} path: packages/*/coverage post_coverage_comment: @@ -106,7 +109,9 @@ jobs: pull-requests: write # For commenting strategy: matrix: - node-version: [22.x] # Reduce noise by only posting the comment once + # Reduce noise by only posting the comment once + os: [ubuntu-latest] + node-version: [22.x] steps: - name: Checkout repository uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 @@ -114,7 +119,7 @@ jobs: - name: Download coverage reports artifact uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 with: - name: coverage-reports-${{ matrix.node-version }} + name: coverage-reports-${{ matrix.node-version }}-${{ matrix.os }} path: coverage_artifact # Download to a specific directory - name: Post Coverage Comment using Composite Action @@ -125,6 +130,7 @@ jobs: cli_full_text_summary_file: coverage_artifact/cli/coverage/full-text-summary.txt core_full_text_summary_file: coverage_artifact/core/coverage/full-text-summary.txt node_version: ${{ matrix.node-version }} + os: ${{ matrix.os }} github_token: ${{ secrets.GITHUB_TOKEN }} codeql: diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index d23b0737..9820ba5f 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -7,28 +7,16 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { GitService } from './gitService.js'; import * as path from 'path'; -import type * as FsPromisesModule from 'fs/promises'; +import * as fs from 'fs/promises'; +import * as os from 'os'; import type { ChildProcess } from 'node:child_process'; +import { getProjectHash, GEMINI_DIR } from '../utils/paths.js'; const hoistedMockExec = vi.hoisted(() => vi.fn()); vi.mock('node:child_process', () => ({ exec: hoistedMockExec, })); -const hoistedMockMkdir = vi.hoisted(() => vi.fn()); -const hoistedMockReadFile = vi.hoisted(() => vi.fn()); -const hoistedMockWriteFile = vi.hoisted(() => vi.fn()); - -vi.mock('fs/promises', async (importOriginal) => { - const actual = (await importOriginal()) as typeof FsPromisesModule; - return { - ...actual, - mkdir: hoistedMockMkdir, - readFile: hoistedMockReadFile, - writeFile: hoistedMockWriteFile, - }; -}); - const hoistedMockEnv = vi.hoisted(() => vi.fn()); const hoistedMockSimpleGit = vi.hoisted(() => vi.fn()); const hoistedMockCheckIsRepo = vi.hoisted(() => vi.fn()); @@ -53,38 +41,30 @@ vi.mock('../utils/gitUtils.js', () => ({ isGitRepository: hoistedIsGitRepositoryMock, })); -const hoistedMockIsNodeError = vi.hoisted(() => vi.fn()); -vi.mock('../utils/errors.js', () => ({ - isNodeError: hoistedMockIsNodeError, -})); - const hoistedMockHomedir = vi.hoisted(() => vi.fn()); -vi.mock('os', () => ({ - homedir: hoistedMockHomedir, -})); - -const hoistedMockCreateHash = vi.hoisted(() => { - const mockUpdate = vi.fn().mockReturnThis(); - const mockDigest = vi.fn(); +vi.mock('os', async (importOriginal) => { + const actual = await importOriginal(); return { - createHash: vi.fn(() => ({ - update: mockUpdate, - digest: mockDigest, - })), - mockUpdate, - mockDigest, + ...actual, + homedir: hoistedMockHomedir, }; }); -vi.mock('crypto', () => ({ - createHash: hoistedMockCreateHash.createHash, -})); describe('GitService', () => { - const mockProjectRoot = '/test/project'; - const mockHomedir = '/mock/home'; - const mockHash = 'mock-hash'; + let testRootDir: string; + let projectRoot: string; + let homedir: string; + let hash: string; + + beforeEach(async () => { + testRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'git-service-test-')); + projectRoot = path.join(testRootDir, 'project'); + homedir = path.join(testRootDir, 'home'); + await fs.mkdir(projectRoot, { recursive: true }); + await fs.mkdir(homedir, { recursive: true }); + + hash = getProjectHash(projectRoot); - beforeEach(() => { vi.clearAllMocks(); hoistedIsGitRepositoryMock.mockReturnValue(true); hoistedMockExec.mockImplementation((command, callback) => { @@ -95,13 +75,8 @@ describe('GitService', () => { } return {}; }); - hoistedMockMkdir.mockResolvedValue(undefined); - hoistedMockReadFile.mockResolvedValue(''); - hoistedMockWriteFile.mockResolvedValue(undefined); - hoistedMockIsNodeError.mockImplementation((e) => e instanceof Error); - hoistedMockHomedir.mockReturnValue(mockHomedir); - hoistedMockCreateHash.mockUpdate.mockReturnThis(); - hoistedMockCreateHash.mockDigest.mockReturnValue(mockHash); + + hoistedMockHomedir.mockReturnValue(homedir); hoistedMockEnv.mockImplementation(() => ({ checkIsRepo: hoistedMockCheckIsRepo, @@ -127,19 +102,20 @@ describe('GitService', () => { }); }); - afterEach(() => { + afterEach(async () => { vi.restoreAllMocks(); + await fs.rm(testRootDir, { recursive: true, force: true }); }); describe('constructor', () => { - it('should successfully create an instance if projectRoot is a Git repository', () => { - expect(() => new GitService(mockProjectRoot)).not.toThrow(); + it('should successfully create an instance', () => { + expect(() => new GitService(projectRoot)).not.toThrow(); }); }); describe('verifyGitAvailability', () => { it('should resolve true if git --version command succeeds', async () => { - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); await expect(service.verifyGitAvailability()).resolves.toBe(true); }); @@ -148,7 +124,7 @@ describe('GitService', () => { callback(new Error('git not found')); return {} as ChildProcess; }); - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); await expect(service.verifyGitAvailability()).resolves.toBe(false); }); }); @@ -159,14 +135,14 @@ describe('GitService', () => { callback(new Error('git not found')); return {} as ChildProcess; }); - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); await expect(service.initialize()).rejects.toThrow( 'Checkpointing is enabled, but Git is not installed. Please install Git or disable checkpointing to continue.', ); }); it('should call setupShadowGitRepository if Git is available', async () => { - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); const setupSpy = vi .spyOn(service, 'setupShadowGitRepository') .mockResolvedValue(undefined); @@ -177,33 +153,34 @@ describe('GitService', () => { }); describe('setupShadowGitRepository', () => { - const repoDir = path.join(mockHomedir, '.gemini', 'history', mockHash); - const hiddenGitIgnorePath = path.join(repoDir, '.gitignore'); - const visibleGitIgnorePath = path.join(mockProjectRoot, '.gitignore'); - const gitConfigPath = path.join(repoDir, '.gitconfig'); + let repoDir: string; + let gitConfigPath: string; - it('should create a .gitconfig file with the correct content', async () => { - const service = new GitService(mockProjectRoot); - await service.setupShadowGitRepository(); - const expectedConfigContent = - '[user]\n name = Gemini CLI\n email = gemini-cli@google.com\n[commit]\n gpgsign = false\n'; - expect(hoistedMockWriteFile).toHaveBeenCalledWith( - gitConfigPath, - expectedConfigContent, - ); + beforeEach(() => { + repoDir = path.join(homedir, GEMINI_DIR, 'history', hash); + gitConfigPath = path.join(repoDir, '.gitconfig'); }); it('should create history and repository directories', async () => { - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); await service.setupShadowGitRepository(); - expect(hoistedMockMkdir).toHaveBeenCalledWith(repoDir, { - recursive: true, - }); + const stats = await fs.stat(repoDir); + expect(stats.isDirectory()).toBe(true); + }); + + it('should create a .gitconfig file with the correct content', async () => { + const service = new GitService(projectRoot); + await service.setupShadowGitRepository(); + + const expectedConfigContent = + '[user]\n name = Gemini CLI\n email = gemini-cli@google.com\n[commit]\n gpgsign = false\n'; + const actualConfigContent = await fs.readFile(gitConfigPath, 'utf-8'); + expect(actualConfigContent).toBe(expectedConfigContent); }); it('should initialize git repo in historyDir if not already initialized', async () => { hoistedMockCheckIsRepo.mockResolvedValue(false); - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); await service.setupShadowGitRepository(); expect(hoistedMockSimpleGit).toHaveBeenCalledWith(repoDir); expect(hoistedMockInit).toHaveBeenCalled(); @@ -211,52 +188,49 @@ describe('GitService', () => { it('should not initialize git repo if already initialized', async () => { hoistedMockCheckIsRepo.mockResolvedValue(true); - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); await service.setupShadowGitRepository(); expect(hoistedMockInit).not.toHaveBeenCalled(); }); it('should copy .gitignore from projectRoot if it exists', async () => { - const gitignoreContent = `node_modules/\n.env`; - hoistedMockReadFile.mockImplementation(async (filePath) => { - if (filePath === visibleGitIgnorePath) { - return gitignoreContent; - } - return ''; - }); - const service = new GitService(mockProjectRoot); + const gitignoreContent = 'node_modules/\n.env'; + const visibleGitIgnorePath = path.join(projectRoot, '.gitignore'); + await fs.writeFile(visibleGitIgnorePath, gitignoreContent); + + const service = new GitService(projectRoot); await service.setupShadowGitRepository(); - expect(hoistedMockReadFile).toHaveBeenCalledWith( - visibleGitIgnorePath, - 'utf-8', - ); - expect(hoistedMockWriteFile).toHaveBeenCalledWith( - hiddenGitIgnorePath, - gitignoreContent, - ); + + const hiddenGitIgnorePath = path.join(repoDir, '.gitignore'); + const copiedContent = await fs.readFile(hiddenGitIgnorePath, 'utf-8'); + expect(copiedContent).toBe(gitignoreContent); + }); + + it('should not create a .gitignore in shadow repo if project .gitignore does not exist', async () => { + const service = new GitService(projectRoot); + await service.setupShadowGitRepository(); + + const hiddenGitIgnorePath = path.join(repoDir, '.gitignore'); + // An empty string is written if the file doesn't exist. + const content = await fs.readFile(hiddenGitIgnorePath, 'utf-8'); + expect(content).toBe(''); }); it('should throw an error if reading projectRoot .gitignore fails with other errors', async () => { - const readError = new Error('Read permission denied'); - hoistedMockReadFile.mockImplementation(async (filePath) => { - if (filePath === visibleGitIgnorePath) { - throw readError; - } - return ''; - }); - hoistedMockIsNodeError.mockImplementation( - (e: unknown): e is NodeJS.ErrnoException => e instanceof Error, - ); + const visibleGitIgnorePath = path.join(projectRoot, '.gitignore'); + // Create a directory instead of a file to cause a read error + await fs.mkdir(visibleGitIgnorePath); - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); + // EISDIR is the expected error code on Unix-like systems await expect(service.setupShadowGitRepository()).rejects.toThrow( - 'Read permission denied', + /EISDIR: illegal operation on a directory, read|EBUSY: resource busy or locked, read/, ); }); it('should make an initial commit if no commits exist in history repo', async () => { hoistedMockCheckIsRepo.mockResolvedValue(false); - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); await service.setupShadowGitRepository(); expect(hoistedMockCommit).toHaveBeenCalledWith('Initial commit', { '--allow-empty': null, @@ -265,7 +239,7 @@ describe('GitService', () => { it('should not make an initial commit if commits already exist', async () => { hoistedMockCheckIsRepo.mockResolvedValue(true); - const service = new GitService(mockProjectRoot); + const service = new GitService(projectRoot); await service.setupShadowGitRepository(); expect(hoistedMockCommit).not.toHaveBeenCalled(); }); diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index b0c7d317..460c871a 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -142,11 +142,9 @@ describe('ShellTool Bug Reproduction', () => { shellTool = new ShellTool(config); const abortSignal = new AbortController().signal; - const result = await shellTool.execute( - { command: 'echo "$GEMINI_CLI"' }, - abortSignal, - () => {}, - ); + const command = + os.platform() === 'win32' ? 'echo %GEMINI_CLI%' : 'echo "$GEMINI_CLI"'; + const result = await shellTool.execute({ command }, abortSignal, () => {}); expect(result.returnDisplay).toBe('1' + os.EOL); });