Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug test flakes #1131

Merged
merged 1 commit into from
Feb 24, 2025
+122 −26
Merged

Debug test flakes #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 33 additions & 7 deletions src/core/Cline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,9 @@ export class Cline {
images?: string[] | undefined,
historyItem?: HistoryItem | undefined,
experiments?: Record<string, boolean>,
startTask = true,
) {
if (!task && !images && !historyItem) {
if (startTask && !task && !images && !historyItem) {
throw new Error("Either historyItem or task/images must be provided")
}

Expand All @@ -153,11 +154,32 @@ export class Cline {
// Initialize diffStrategy based on current state
this.updateDiffStrategy(Experiments.isEnabled(experiments ?? {}, EXPERIMENT_IDS.DIFF_STRATEGY))

if (task || images) {
this.startTask(task, images)
} else if (historyItem) {
this.resumeTaskFromHistory()
if (startTask) {
if (task || images) {
this.startTask(task, images)
} else if (historyItem) {
this.resumeTaskFromHistory()
} else {
throw new Error("Either historyItem or task/images must be provided")
}
}
}

static create(...args: ConstructorParameters<typeof Cline>): [Cline, Promise<void>] {
args[10] = false // startTask
const instance = new Cline(...args)

let task

if (args[6] || args[7]) {
task = instance.startTask(args[6], args[7])
} else if (args[8]) {
Comment on lines +169 to +176
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These hardcoded indexes feel pretty fragile / easy for us to forget about next time we add a param to the constructor. Think there's anything we could do to keep them in sync? Maybe a comment at the very least?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "right" solution seems like using an actual type. I can do that as a follow-up right now; most of the new Cline() calls are in tests so it should be pretty safe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is only used by tests at the moment, so I don't think we're risking breaking much in the meantime.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1139

task = instance.resumeTaskFromHistory()
} else {
throw new Error("Either historyItem or task/images must be provided")
}

return [instance, task]
}

// Add method to update diffStrategy
Expand Down Expand Up @@ -745,8 +767,12 @@ export class Cline {
}
}

async abortTask() {
async abortTask(isAbandoned = false) {
// Will stop any autonomously running promises.
if (isAbandoned) {
this.abandoned = true
}

this.abort = true

this.terminalManager.disposeAll()
Expand Down Expand Up @@ -2967,7 +2993,7 @@ export class Cline {
}

// need to call here in case the stream was aborted
if (this.abort) {
if (this.abort || this.abandoned) {
throw new Error("Roo Code instance aborted")
}

Expand Down
102 changes: 87 additions & 15 deletions src/core/__tests__/Cline.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// npx jest src/core/__tests__/Cline.test.ts

import { Cline } from "../Cline"
import { ClineProvider } from "../webview/ClineProvider"
import { ApiConfiguration, ModelInfo } from "../../shared/api"
Expand Down Expand Up @@ -324,8 +326,8 @@ describe("Cline", () => {
})

describe("constructor", () => {
it("should respect provided settings", () => {
const cline = new Cline(
it("should respect provided settings", async () => {
const [cline, task] = Cline.create(
mockProvider,
mockApiConfig,
"custom instructions",
Expand All @@ -337,10 +339,13 @@ describe("Cline", () => {

expect(cline.customInstructions).toBe("custom instructions")
expect(cline.diffEnabled).toBe(false)

await cline.abortTask(true)
await task.catch(() => {})
})

it("should use default fuzzy match threshold when not provided", () => {
const cline = new Cline(
it("should use default fuzzy match threshold when not provided", async () => {
const [cline, task] = await Cline.create(
mockProvider,
mockApiConfig,
"custom instructions",
Expand All @@ -353,12 +358,15 @@ describe("Cline", () => {
expect(cline.diffEnabled).toBe(true)
// The diff strategy should be created with default threshold (1.0)
expect(cline.diffStrategy).toBeDefined()

await cline.abortTask(true)
await task.catch(() => {})
})

it("should use provided fuzzy match threshold", () => {
it("should use provided fuzzy match threshold", async () => {
const getDiffStrategySpy = jest.spyOn(require("../diff/DiffStrategy"), "getDiffStrategy")

const cline = new Cline(
const [cline, task] = Cline.create(
mockProvider,
mockApiConfig,
"custom instructions",
Expand All @@ -373,12 +381,15 @@ describe("Cline", () => {
expect(getDiffStrategySpy).toHaveBeenCalledWith("claude-3-5-sonnet-20241022", 0.9, false)

getDiffStrategySpy.mockRestore()

await cline.abortTask(true)
await task.catch(() => {})
})

it("should pass default threshold to diff strategy when not provided", () => {
it("should pass default threshold to diff strategy when not provided", async () => {
const getDiffStrategySpy = jest.spyOn(require("../diff/DiffStrategy"), "getDiffStrategy")

const cline = new Cline(
const [cline, task] = Cline.create(
mockProvider,
mockApiConfig,
"custom instructions",
Expand All @@ -393,6 +404,9 @@ describe("Cline", () => {
expect(getDiffStrategySpy).toHaveBeenCalledWith("claude-3-5-sonnet-20241022", 1.0, false)

getDiffStrategySpy.mockRestore()

await cline.abortTask(true)
await task.catch(() => {})
})

it("should require either task or historyItem", () => {
Expand Down Expand Up @@ -455,7 +469,15 @@ describe("Cline", () => {
})

it("should include timezone information in environment details", async () => {
const cline = new Cline(mockProvider, mockApiConfig, undefined, false, false, undefined, "test task")
const [cline, task] = Cline.create(
mockProvider,
mockApiConfig,
undefined,
false,
false,
undefined,
"test task",
)

const details = await cline["getEnvironmentDetails"](false)

Expand All @@ -464,11 +486,24 @@ describe("Cline", () => {
expect(details).toMatch(/UTC-7:00/) // Fixed offset for America/Los_Angeles
expect(details).toContain("# Current Time")
expect(details).toMatch(/1\/1\/2024.*5:00:00 AM.*\(America\/Los_Angeles, UTC-7:00\)/) // Full time string format

await cline.abortTask(true)
await task.catch(() => {})
})

describe("API conversation handling", () => {
it("should clean conversation history before sending to API", async () => {
const cline = new Cline(mockProvider, mockApiConfig, undefined, false, false, undefined, "test task")
const [cline, task] = Cline.create(
mockProvider,
mockApiConfig,
undefined,
false,
false,
undefined,
"test task",
)
cline.abandoned = true
await task

// Mock the API's createMessage method to capture the conversation history
const createMessageSpy = jest.fn()
Expand Down Expand Up @@ -576,7 +611,7 @@ describe("Cline", () => {
]

// Test with model that supports images
const clineWithImages = new Cline(
const [clineWithImages, taskWithImages] = Cline.create(
mockProvider,
configWithImages,
undefined,
Expand All @@ -585,6 +620,7 @@ describe("Cline", () => {
undefined,
"test task",
)

// Mock the model info to indicate image support
jest.spyOn(clineWithImages.api, "getModel").mockReturnValue({
id: "claude-3-sonnet",
Expand All @@ -598,10 +634,11 @@ describe("Cline", () => {
outputPrice: 0.75,
} as ModelInfo,
})

clineWithImages.apiConversationHistory = conversationHistory

// Test with model that doesn't support images
const clineWithoutImages = new Cline(
const [clineWithoutImages, taskWithoutImages] = Cline.create(
mockProvider,
configWithoutImages,
undefined,
Expand All @@ -610,6 +647,7 @@ describe("Cline", () => {
undefined,
"test task",
)

// Mock the model info to indicate no image support
jest.spyOn(clineWithoutImages.api, "getModel").mockReturnValue({
id: "gpt-3.5-turbo",
Expand All @@ -623,6 +661,7 @@ describe("Cline", () => {
outputPrice: 0.2,
} as ModelInfo,
})

clineWithoutImages.apiConversationHistory = conversationHistory

// Mock abort state for both instances
Expand All @@ -631,6 +670,7 @@ describe("Cline", () => {
set: () => {},
configurable: true,
})

Object.defineProperty(clineWithoutImages, "abort", {
get: () => false,
set: () => {},
Expand All @@ -645,6 +685,7 @@ describe("Cline", () => {
content,
"",
])

// Set up mock streams
const mockStreamWithImages = (async function* () {
yield { type: "text", text: "test response" }
Expand Down Expand Up @@ -672,6 +713,12 @@ describe("Cline", () => {
},
]

clineWithImages.abandoned = true
await taskWithImages.catch(() => {})

clineWithoutImages.abandoned = true
await taskWithoutImages.catch(() => {})

// Trigger API requests
await clineWithImages.recursivelyMakeClineRequests([{ type: "text", text: "test request" }])
await clineWithoutImages.recursivelyMakeClineRequests([{ type: "text", text: "test request" }])
Expand All @@ -695,7 +742,15 @@ describe("Cline", () => {
})

it.skip("should handle API retry with countdown", async () => {
const cline = new Cline(mockProvider, mockApiConfig, undefined, false, false, undefined, "test task")
const [cline, task] = Cline.create(
mockProvider,
mockApiConfig,
undefined,
false,
false,
undefined,
"test task",
)

// Mock delay to track countdown timing
const mockDelay = jest.fn().mockResolvedValue(undefined)
Expand Down Expand Up @@ -809,10 +864,21 @@ describe("Cline", () => {
expect(errorMessage).toBe(
`${mockError.message}\n\nRetry attempt 1\nRetrying in ${baseDelay} seconds...`,
)

await cline.abortTask(true)
await task.catch(() => {})
})

it.skip("should not apply retry delay twice", async () => {
const cline = new Cline(mockProvider, mockApiConfig, undefined, false, false, undefined, "test task")
const [cline, task] = Cline.create(
mockProvider,
mockApiConfig,
undefined,
false,
false,
undefined,
"test task",
)

// Mock delay to track countdown timing
const mockDelay = jest.fn().mockResolvedValue(undefined)
Expand Down Expand Up @@ -925,11 +991,14 @@ describe("Cline", () => {
undefined,
false,
)

await cline.abortTask(true)
await task.catch(() => {})
})

describe("loadContext", () => {
it("should process mentions in task and feedback tags", async () => {
const cline = new Cline(
const [cline, task] = Cline.create(
mockProvider,
mockApiConfig,
undefined,
Expand Down Expand Up @@ -1002,6 +1071,9 @@ describe("Cline", () => {
const toolResult2 = processedContent[3] as Anthropic.ToolResultBlockParam
const content2 = Array.isArray(toolResult2.content) ? toolResult2.content[0] : toolResult2.content
expect((content2 as Anthropic.TextBlockParam).text).toBe("Regular tool result with @/path")

await cline.abortTask(true)
await task.catch(() => {})
})
})
})
Expand Down
6 changes: 2 additions & 4 deletions src/core/diff/strategies/new-unified/__tests__/edit-strategies.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
/// <reference types="jest" />

import { applyContextMatching, applyDMP, applyGitFallback } from "../edit-strategies"
import { Hunk } from "../types"

Expand Down Expand Up @@ -277,7 +275,7 @@ describe("applyGitFallback", () => {
expect(result.result.join("\n")).toEqual("line1\nnew line2\nline3")
expect(result.confidence).toBe(1)
expect(result.strategy).toBe("git-fallback")
}, 10_000)
})

it("should return original content with 0 confidence when changes cannot be applied", async () => {
const hunk = {
Expand All @@ -293,5 +291,5 @@ describe("applyGitFallback", () => {
expect(result.result).toEqual(content)
expect(result.confidence).toBe(0)
expect(result.strategy).toBe("git-fallback")
}, 10_000)
})
})