From 0923c8b47e53a26560e13e908f57f9dbe871f251 Mon Sep 17 00:00:00 2001 From: Brace Sproul Date: Sun, 27 Jul 2025 12:11:34 -0700 Subject: [PATCH] fix: running tests and in ci (#536) * fix: running tests and in ci * cr * cr * cr * exit 0 when no tests * cr --- .github/workflows/unit-tests.yml | 40 ++++++++++++++++++++++ apps/cli/package.json | 2 +- apps/open-swe/jest.config.js | 3 ++ apps/open-swe/package.json | 4 +-- apps/open-swe/src/__tests__/retry.test.ts | 40 ++++++++++++---------- apps/open-swe/src/__tests__/tokens.test.ts | 10 +++--- apps/open-swe/src/utils/tokens.ts | 12 +++---- apps/open-swe/tsconfig.json | 3 +- package.json | 3 +- turbo.json | 3 ++ 10 files changed, 86 insertions(+), 34 deletions(-) create mode 100644 .github/workflows/unit-tests.yml diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml new file mode 100644 index 00000000..d8bff89a --- /dev/null +++ b/.github/workflows/unit-tests.yml @@ -0,0 +1,40 @@ +# This workflow will run unit tests for the current project + +name: Unit Tests +permissions: + contents: read +on: + push: + branches: ["main"] + pull_request: + workflow_dispatch: # Allows triggering the workflow manually in GitHub UI + +# If another push to the same PR or branch happens while this workflow is still running, +# cancel the earlier run in favor of the next run. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + unit-tests: + name: Unit Tests + strategy: + matrix: + os: [ubuntu-latest] + node-version: [18.x, 20.x] + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@v4 + - name: Enable Corepack + run: corepack enable + - name: Use Node.js ${{ matrix.node-version }} + uses: actions/setup-node@v3 + with: + node-version: ${{ matrix.node-version }} + cache: "yarn" + - name: Install dependencies + run: yarn install --immutable --mode=skip-build + - name: Build project + run: yarn build + - name: Run tests + run: yarn test diff --git a/apps/cli/package.json b/apps/cli/package.json index b77a6b8b..e08f6fcf 100644 --- a/apps/cli/package.json +++ b/apps/cli/package.json @@ -12,7 +12,7 @@ "lint:fix": "eslint . --fix", "format": "prettier --write .", "format:check": "prettier --check .", - "test": "echo \"Error: no test specified\" && exit 1", + "test": "echo \"Error: no test specified\" && exit 0", "dev": "tsx src/index.tsx" }, "dependencies": { diff --git a/apps/open-swe/jest.config.js b/apps/open-swe/jest.config.js index 9e893743..219fab5c 100644 --- a/apps/open-swe/jest.config.js +++ b/apps/open-swe/jest.config.js @@ -2,6 +2,8 @@ export default { preset: "ts-jest/presets/default-esm", moduleNameMapper: { "^(\\.{1,2}/.*)\\.js$": "$1", + "^@open-swe/shared$": "/../../packages/shared/src/index.ts", + "^@open-swe/shared/(.*)$": "/../../packages/shared/src/$1", }, transform: { "^.+\\.tsx?$": [ @@ -15,4 +17,5 @@ export default { setupFiles: ["dotenv/config"], passWithNoTests: true, testTimeout: 20_000, + testMatch: ["/src/**/*.test.ts"], }; diff --git a/apps/open-swe/package.json b/apps/open-swe/package.json index 720d0d30..c23e293e 100644 --- a/apps/open-swe/package.json +++ b/apps/open-swe/package.json @@ -16,8 +16,8 @@ "lint:fix": "eslint . --fix", "format": "prettier --write .", "format:check": "prettier --check .", - "test": "node --experimental-vm-modules node_modules/jest/bin/jest.js --testPathPattern=\\.test\\.ts$ --testPathIgnorePatterns=\\.int\\.test\\.ts$", - "test:int": "node --experimental-vm-modules node_modules/jest/bin/jest.js --testPathPattern=\\.int\\.test\\.ts$", + "test": "NODE_OPTIONS=--experimental-vm-modules yarn run jest --config jest.config.js --testPathIgnorePatterns=int.test.ts", + "test:int": "node --experimental-vm-modules node_modules/jest/bin/jest.js --config jest.config.js --testPathPattern=int.test.ts", "test:single": "NODE_OPTIONS=--experimental-vm-modules yarn run jest --config jest.config.js --testTimeout 100000", "eval:single": "NODE_OPTIONS=--experimental-vm-modules yarn run vitest --config ls.vitest.config.ts --run", "postinstall": "turbo build" diff --git a/apps/open-swe/src/__tests__/retry.test.ts b/apps/open-swe/src/__tests__/retry.test.ts index f4954ea6..d92c91a6 100644 --- a/apps/open-swe/src/__tests__/retry.test.ts +++ b/apps/open-swe/src/__tests__/retry.test.ts @@ -31,7 +31,9 @@ describe("withRetry", () => { .fn<() => Promise>() .mockRejectedValue(new Error("always fails")); - await expect(withRetry(mockFn)).rejects.toThrow("always fails"); + const result = await withRetry(mockFn); + expect(result).toBeInstanceOf(Error); + expect((result as Error).message).toBe("always fails"); expect(mockFn).toHaveBeenCalledTimes(4); // 1 initial + 3 retries }); @@ -40,9 +42,9 @@ describe("withRetry", () => { .fn<() => Promise>() .mockRejectedValue(new Error("always fails")); - await expect(withRetry(mockFn, { retries: 2 })).rejects.toThrow( - "always fails", - ); + const result = await withRetry(mockFn, { retries: 2 }); + expect(result).toBeInstanceOf(Error); + expect((result as Error).message).toBe("always fails"); expect(mockFn).toHaveBeenCalledTimes(3); // 1 initial + 2 retries }); @@ -52,11 +54,11 @@ describe("withRetry", () => { .mockRejectedValue(new Error("always fails")); const startTime = Date.now(); - await expect(withRetry(mockFn, { retries: 2, delay: 100 })).rejects.toThrow( - "always fails", - ); + const result = await withRetry(mockFn, { retries: 2, delay: 100 }); const endTime = Date.now(); + expect(result).toBeInstanceOf(Error); + expect((result as Error).message).toBe("always fails"); expect(mockFn).toHaveBeenCalledTimes(3); expect(endTime - startTime).toBeGreaterThanOrEqual(200); // 2 delays of 100ms each }); @@ -67,11 +69,11 @@ describe("withRetry", () => { .mockRejectedValue(new Error("always fails")); const startTime = Date.now(); - await expect(withRetry(mockFn, { retries: 2 })).rejects.toThrow( - "always fails", - ); + const result = await withRetry(mockFn, { retries: 2 }); const endTime = Date.now(); + expect(result).toBeInstanceOf(Error); + expect((result as Error).message).toBe("always fails"); expect(mockFn).toHaveBeenCalledTimes(3); expect(endTime - startTime).toBeLessThan(50); // Should be very fast with no delay }); @@ -81,13 +83,13 @@ describe("withRetry", () => { .fn<() => Promise>() .mockRejectedValue("string error"); - await expect(withRetry(mockFn, { retries: 1 })).rejects.toThrow( - "string error", - ); + const result = await withRetry(mockFn, { retries: 1 }); + expect(result).toBeInstanceOf(Error); + expect((result as Error).message).toBe("string error"); expect(mockFn).toHaveBeenCalledTimes(2); }); - it("should throw the last error after all retries", async () => { + it("should return the last error after all retries", async () => { const error1 = new Error("first error"); const error2 = new Error("second error"); const lastError = new Error("last error"); @@ -98,9 +100,9 @@ describe("withRetry", () => { .mockRejectedValueOnce(error2) .mockRejectedValue(lastError); - await expect(withRetry(mockFn, { retries: 2 })).rejects.toThrow( - "last error", - ); + const result = await withRetry(mockFn, { retries: 2 }); + expect(result).toBeInstanceOf(Error); + expect((result as Error).message).toBe("last error"); expect(mockFn).toHaveBeenCalledTimes(3); }); @@ -140,7 +142,9 @@ describe("createRetryWrapper", () => { const wrappedFn = createRetryWrapper(originalFn, { retries: 1 }); - await expect(wrappedFn()).rejects.toThrow("always fails"); + const result = await wrappedFn(); + expect(result).toBeInstanceOf(Error); + expect((result as Error).message).toBe("always fails"); expect(originalFn).toHaveBeenCalledTimes(2); // 1 initial + 1 retry }); diff --git a/apps/open-swe/src/__tests__/tokens.test.ts b/apps/open-swe/src/__tests__/tokens.test.ts index 6863dc73..4239d3e6 100644 --- a/apps/open-swe/src/__tests__/tokens.test.ts +++ b/apps/open-swe/src/__tests__/tokens.test.ts @@ -6,7 +6,7 @@ import { MAX_INTERNAL_TOKENS, } from "../utils/tokens.js"; -describe("calculateConversationHistoryTokenCount", async () => { +describe("calculateConversationHistoryTokenCount", () => { it("should return 0 for empty messages array", async () => { const result = calculateConversationHistoryTokenCount([]); expect(result).toBe(0); @@ -124,8 +124,8 @@ describe("calculateConversationHistoryTokenCount", async () => { }); expect(resultWithoutOption).toBeGreaterThan(resultWithOption); - // First two messages should be ~7 tokens - expect(resultWithOption).toBe(7); + // First two messages should be ~8 tokens + expect(resultWithOption).toBe(8); }); it("should not separate AI messages with tool calls from their tool messages when excluding from end", async () => { @@ -214,7 +214,7 @@ describe("calculateConversationHistoryTokenCount", async () => { }); }); -describe("getMessagesSinceLastSummary", async () => { +describe("getMessagesSinceLastSummary", () => { it("should return all messages when there is no summary message", async () => { const messages = [ new HumanMessage({ content: "Message 1" }), @@ -632,7 +632,7 @@ describe("getMessagesSinceLastSummary", async () => { }); }); -describe("MAX_INTERNAL_TOKENS constant", async () => { +describe("MAX_INTERNAL_TOKENS constant", () => { it("should be defined as 60,000", async () => { expect(MAX_INTERNAL_TOKENS).toBe(60_000); }); diff --git a/apps/open-swe/src/utils/tokens.ts b/apps/open-swe/src/utils/tokens.ts index 9d7a6992..5a84a9f8 100644 --- a/apps/open-swe/src/utils/tokens.ts +++ b/apps/open-swe/src/utils/tokens.ts @@ -17,7 +17,7 @@ export function calculateConversationHistoryTokenCount( excludeCountFromEnd?: number; }, ) { - let totalChars = 0; + let totalTokens = 0; let messagesToCount = messages; if (options?.excludeCountFromEnd && options.excludeCountFromEnd > 0) { @@ -33,25 +33,25 @@ export function calculateConversationHistoryTokenCount( if (isHumanMessage(m) || isToolMessage(m)) { const contentString = getMessageContentString(m.content); // Divide each char by 4 as it's roughly one token per 4 characters. - totalChars += contentString.length / 4; + totalTokens += Math.ceil(contentString.length / 4); } if (isAIMessage(m)) { const usageMetadata = m.usage_metadata; if (usageMetadata) { - totalChars += usageMetadata.output_tokens; + totalTokens += usageMetadata.total_tokens; } else { const contentString = getMessageContentString(m.content); - totalChars += contentString.length / 4; + totalTokens += Math.ceil(contentString.length / 4); m.tool_calls?.forEach((tc) => { const nameAndArgs = tc.name + JSON.stringify(tc.args); - totalChars += nameAndArgs.length / 4; + totalTokens += Math.ceil(nameAndArgs.length / 4); }); } } }); - return totalChars; + return totalTokens; } /** diff --git a/apps/open-swe/tsconfig.json b/apps/open-swe/tsconfig.json index ea71a584..e1282309 100644 --- a/apps/open-swe/tsconfig.json +++ b/apps/open-swe/tsconfig.json @@ -18,7 +18,8 @@ "strictFunctionTypes": false, "outDir": "dist", "types": ["jest", "node"], - "resolveJsonModule": true + "resolveJsonModule": true, + "isolatedModules": true }, "include": ["**/*.ts", "**/*.js", "jest.setup.cjs"], "exclude": ["node_modules", "dist"] diff --git a/package.json b/package.json index 2226404e..da8ecc68 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,8 @@ "format": "turbo format", "format:check": "turbo format:check", "lint": "turbo lint", - "lint:fix": "turbo lint:fix" + "lint:fix": "turbo lint:fix", + "test": "turbo test" }, "devDependencies": { "turbo": "^2.5.0", diff --git a/turbo.json b/turbo.json index fa8a462f..c3b9113c 100644 --- a/turbo.json +++ b/turbo.json @@ -20,6 +20,9 @@ }, "dev": { "dependsOn": ["^dev"] + }, + "test": { + "dependsOn": ["^test"] } } }