Skip to content
This repository was archived by the owner on Aug 1, 2025. It is now read-only.

add: .cody/ignore integration tests in agent#2963

Merged
abeatrix merged 17 commits into
mainfrom
bee/a-test
Feb 5, 2024
Merged

add: .cody/ignore integration tests in agent#2963
abeatrix merged 17 commits into
mainfrom
bee/a-test

Conversation

@abeatrix

@abeatrix abeatrix commented Jan 31, 2024

Copy link
Copy Markdown
Contributor

Part of https://github.com/sourcegraph/cody/issues/2920

Summary:

  • Adding a new check/isCodyIgnoredFile request handler to check if a file path is being ignored by Cody
  • Updating tests to use this new request instead of the isCodyIgnoredFile utility method
  • Adding a test to confirm .cody/ignore is working on startup

This PR adds integration tests for the following features to agent:

Local context source covered in the test:

Also updated to allow remote context which should be filtered during sync time (https://github.com/sourcegraph/sourcegraph/pull/59836) to allow support for multi-repo context in clients.

Follow-Up Works

  • Add assertions for checking input context during Network Request

Test plan

Green bots means 👍

// Ignore all non-file URIs
// Remote context (e.g. unified, multi-repo) are filtered during sync time,
// and not ignored by clients.
if (uri.scheme === 'https') {

@abeatrix abeatrix Jan 31, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Without this, the 'chat/submitMessage (addEnhancedContext: true, multi-repo test)' test would fail as we will remove ALL context returned from the remote host. Thanks olaf for spotting this!

@abeatrix abeatrix changed the title wip add(wip): .cody/ignore integration tests to agent Jan 31, 2024
@abeatrix abeatrix requested review from a team, DanTup, olafurpg and pkukielka January 31, 2024 21:02
@abeatrix abeatrix changed the title add(wip): .cody/ignore integration tests to agent add: .cody/ignore integration tests to agent Feb 1, 2024
@abeatrix abeatrix changed the title add: .cody/ignore integration tests to agent add: .cody/ignore integration tests in agent Feb 1, 2024
@chwarwick

Copy link
Copy Markdown
Contributor

@abeatrix This will be great to get tests in place for. Currently this is validating that the ignore worked by asserting on the outputs, however what we are trying to protect against is adding ignored content to the input. This snippet shows how you can get access to the requests that were sent. Adding assertions to the content sent to the llm would provide a much higher level of confidence with this feature.

@abeatrix

abeatrix commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

@abeatrix This will be great to get tests in place for. Currently this is validating that the ignore worked by asserting on the outputs, however what we are trying to protect against is adding ignored content to the input. This snippet shows how you can get access to the requests that were sent. Adding assertions to the content sent to the llm would provide a much higher level of confidence with this feature.

Thanks for pointing me in the right direction to test this feature reliably! I will work on adding assertions to check the content sent in network requests.

@chwarwick should I keep what I have here now for asserting on the output, or do you think it'd be unnecessary?

@chwarwick

Copy link
Copy Markdown
Contributor

@chwarwick should I keep what I have here now for asserting on the output, or do you think it'd be unnecessary?

I think they good to leave since it covers the expectations of what the output will be including the post processing steps. For example if running autocomplete in an ignored file started returning errors instead of empty responses that would be visible.

Comment thread agent/src/index.test.ts
// Join all the string from each groupedMsgs[] together into
// one block of text, and then check if it contains the ignored file name
// to confirm context from the ignored file was not sent to the server.
const groupedText = groupedMsgs.flat().join(' ')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chwarwick Would this approach work? 😅

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure, if we find something better in the future we can update it. As long as we know it catches the failure condition I think it's a positive.

@abeatrix abeatrix requested review from a team and chwarwick February 1, 2024 22:02

@DanTup DanTup left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks reasonable to me, but I'm not too familiar with the code or the agent tests so I'll defer to others for this :)

Comment thread agent/src/index.test.ts Outdated
@abeatrix abeatrix requested a review from DanTup February 2, 2024 23:21
@abeatrix abeatrix merged commit 389cbf2 into main Feb 5, 2024
@abeatrix abeatrix deleted the bee/a-test branch February 5, 2024 16:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants