Skip to content

Commit

Permalink
Agent: print out helpful error message on missing recording (#3197)
Browse files Browse the repository at this point in the history
Previously, it could be difficult to debug why an agent test case was
failing due to missing HTTP recordings. In many cases, the failure
happened because 1) we leaked an absolute path into the recording or 2)
we forgot to sort context files before constructing the prompt. This PR
is an attempt to make it easier to debug failures in this cases by

* Taking the body and URL of the HTTP request with missing recording
* Find the entry in the recording file with matching URL and closest
levenshein distance from the body
* Prints out a unified diff of the multi-line formatted JSON bodies


## Test plan

Green CI.

Manually tested by making modifications to the recording file. The
output looks like this

![CleanShot 2024-02-16 at 15 20
02@2x](https://github.com/sourcegraph/cody/assets/1408093/41a7e0b6-a169-42d8-9cb4-20d5c6e1a130)



<!-- Required. See
https://sourcegraph.com/docs/dev/background-information/testing_principles.
-->
  • Loading branch information
olafurpg authored Feb 16, 2024
1 parent 5a638a1 commit 155fa83
Show file tree
Hide file tree
Showing 8 changed files with 138 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ interface CodyAgentServer {
fun testing_networkRequests(params: Null): CompletableFuture<Testing_NetworkRequestsResult>
@JsonRequest("testing/requestErrors")
fun testing_requestErrors(params: Null): CompletableFuture<Testing_RequestErrorsResult>
@JsonRequest("testing/closestPostData")
fun testing_closestPostData(params: Testing_ClosestPostDataParams): CompletableFuture<Testing_ClosestPostDataResult>
@JsonRequest("testing/progressCancelation")
fun testing_progressCancelation(params: Testing_ProgressCancelationParams): CompletableFuture<Testing_ProgressCancelationResult>
@JsonRequest("testing/reset")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
@file:Suppress("FunctionName", "ClassName")
package com.sourcegraph.cody.protocol_generated

data class Testing_ClosestPostDataParams(
var url: String? = null,
var postData: String? = null,
)

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
@file:Suppress("FunctionName", "ClassName")
package com.sourcegraph.cody.protocol_generated

data class Testing_ClosestPostDataResult(
var closestBody: String? = null,
)

10 changes: 8 additions & 2 deletions agent/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,26 +24,32 @@
},
"dependencies": {
"@pollyjs/core": "^6.0.6",
"@pollyjs/persister": "^6.0.6",
"@pollyjs/persister-fs": "^6.0.6",
"@sourcegraph/cody-shared": "workspace:*",
"@sourcegraph/telemetry": "^0.16.0",
"@types/js-levenshtein": "^1.1.1",
"commander": "^11.1.0",
"csv-writer": "^1.6.0",
"dedent": "^0.7.0",
"env-paths": "^3.0.0",
"fast-myers-diff": "^3.1.0",
"js-levenshtein": "^1.1.6",
"minimatch": "^9.0.3",
"pretty-ms": "^8.0.0",
"uuid": "^9.0.0",
"vscode-uri": "^3.0.7"
},
"devDependencies": {
"google-protobuf": "^3.21.2",
"@types/google-protobuf": "3.15.12",
"@types/dedent": "^0.7.0",
"@types/diff": "^5.0.9",
"@types/google-protobuf": "3.15.12",
"@types/minimatch": "^5.1.2",
"@types/uuid": "^9.0.2",
"@types/vscode": "^1.80.0",
"diff": "^5.2.0",
"esbuild": "^0.18.19",
"google-protobuf": "^3.21.2",
"parse-git-diff": "^0.0.14",
"pkg": "^5.8.1",
"rimraf": "^5.0.5"
Expand Down
61 changes: 61 additions & 0 deletions agent/src/TestClient.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import assert from 'assert'

import { createPatch } from 'diff'

import { spawn, type ChildProcessWithoutNullStreams } from 'child_process'
import fspromises from 'fs/promises'
import os from 'os'
Expand All @@ -16,6 +19,7 @@ import type {
DeleteFileOperation,
EditTask,
ExtensionConfiguration,
NetworkRequest,
ProgressReportParams,
ProgressStartParams,
ProtocolCodeLens,
Expand Down Expand Up @@ -507,13 +511,70 @@ export class TestClient extends MessageHandler {
return { panelID: id, transcript: reply, lastMessage: reply.messages.at(-1) }
}

// Given the following missing recording, tries to find an existing
// recording that has the closest levenshtein distance and prints out a
// unified diff. This could save a lot of time trying to debug a test
// failure caused by missing recordings for common scenarios like 1) leaking
// an absolute file path into the prompt or 2) forgetting to sort context
// files.
private async printDiffAgainstClosestMatchingRecording(
missingRecording: NetworkRequest
): Promise<void> {
const message = missingRecording.error ?? ''
const jsonText = message.split('\n').slice(1).join('\n')
const json = JSON.parse(jsonText)
const bodyText = json?.body ?? '{}'
const body = JSON.parse(bodyText)
const { closestBody } = await this.request('testing/closestPostData', {
url: json?.url ?? '',
postData: bodyText,
})
if (closestBody) {
const oldChange = JSON.stringify(body, null, 2)
const newChange = JSON.stringify(JSON.parse(closestBody), null, 2)
if (oldChange === newChange) {
console.log(
dedent`There exists a recording with exactly the same request body, but for some reason the recordings did not match.
This only really happens in exceptional cases like
- There is a bug in how Polly computes HTTP request identifiers
- Somebody manually edited the HTTP recording file
Possible ways to fix the problem:
- Confirm tests run in passthrough mode: CODY_RECORDING_MODE=passthrough pnpm test agent/src/index.test.ts
- Reset recordings and re-record everything: rm -rf agent/recordings && pnpm update-agent-recordings
`
)
} else {
const patch = createPatch(
missingRecording.url,
oldChange,
newChange,
'the request in this test that has no matching recording',
'the closest matchin recording in the recording file.'
)
console.log(
`
Found a recording in the recording file that looks similar to this request that has no matching recording.
Sometimes this happens when our prompt construction logic is non-determinic. For example, if we expose
an absolute file path in the recording, then the tests fail in CI because the absolutely file path in CI
is different from the one in the recording file. Another example, sometimes the ordering of context files
is non-deterministic resulting in failing tests in CI because the ordering of context files in CI is different.
Closely inspect the diff below to non-determinic prompt construction is the reason behind this failure.
${patch}`
)
}
}
}

public async shutdownAndExit() {
if (this.isAlive()) {
const { errors } = await this.request('testing/requestErrors', null)
const missingRecordingErrors = errors.filter(({ error }) =>
error?.includes?.('`recordIfMissing` is')
)
if (missingRecordingErrors.length > 0) {
for (const error of missingRecordingErrors) {
await this.printDiffAgainstClosestMatchingRecording(error)
}
const errorMessage = missingRecordingErrors[0].error?.split?.('\n')?.[0]
throw new Error(
dedent`${errorMessage}.
Expand Down
24 changes: 24 additions & 0 deletions agent/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ import { emptyEvent } from '../../vscode/src/testutils/emptyEvent'
import type { PollyRequestError } from './cli/jsonrpc'
import { AgentWorkspaceEdit } from '../../vscode/src/testutils/AgentWorkspaceEdit'
import type { CompletionItemID } from '../../vscode/src/completions/logger'
import type { Har } from '@pollyjs/persister'
import levenshtein from 'js-levenshtein'

const inMemorySecretStorageMap = new Map<string, string>()
const globalState = new AgentGlobalState()
Expand Down Expand Up @@ -393,6 +395,28 @@ export class Agent extends MessageHandler {
requests: requests.map(req => ({ url: req.url, body: req.body })),
}
})
this.registerAuthenticatedRequest('testing/closestPostData', async ({ url, postData }) => {
const polly = this.params?.polly
let closestDistance = Number.MAX_VALUE
let closest = ''
if (polly) {
const persister = polly.persister._cache as Map<string, Promise<Har>>
for (const [, har] of persister) {
for (const entry of (await har).log.entries) {
if (entry.request.url !== url) {
continue
}
const entryPostData = entry.request.postData?.text ?? ''
const distance = levenshtein(postData, entryPostData)
if (distance < closestDistance) {
closest = entryPostData
closestDistance = distance
}
}
}
}
return { closestBody: closest }
})
this.registerAuthenticatedRequest('testing/requestErrors', async () => {
const requests = this.params?.requestErrors ?? []
return { errors: requests.map(({ request, error }) => ({ url: request.url, error })) }
Expand Down
Loading

0 comments on commit 155fa83

Please sign in to comment.