-
Notifications
You must be signed in to change notification settings - Fork 13
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.
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
Integration Test framework and an initial test for Document Code #1291
Conversation
We now have an environment-variable based feature flag, so the setting is no longer needed nor helpful.
figured out how to run the tests on the JUnit runner thread
LLM interaction is not simulated yet, but coming soon
It had disappeared while re-threading things for integration tests
required killing off the old agent process, which fixed some startup errors. More work to do
Issue was that Agent should not be in integrationi-test mode
There was a naming conflict before that has been resolved
@@ -97,10 +97,12 @@ private constructor( | |||
val conn = startAgentProcess() | |||
val client = CodyAgentClient() | |||
client.onSetConfigFeatures = project.service<CurrentConfigFeatures>() | |||
logger.warn("Starting json-rpc Launcher") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't all that logs be on the info
level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few extra logging statements at key lifecycle stages of starting the Agent, because I found it tremendously helpful in narrowing down where it was failing when the Agent is pooping out. Which is often. I'll review them all, though, to see which ones we should really keep.
@@ -156,12 +159,27 @@ private constructor( | |||
processBuilder.environment()["CODY_LOG_EVENT_MODE"] = "connected-instance-only" | |||
} | |||
|
|||
if (ConfigUtil.isIntegrationTestModeEnabled()) { | |||
// N.B. Do not set CODY_TESTING=true -- that is for Agent-side tests. | |||
val testToken = System.getenv("CODY_INTEGRATION_TEST_TOKEN") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should reuse tokens and polly.js recording infrastructure we already have in the agent.
This way we won't need to confugure/understand/setup two independent testing environments, and all token management could be taken care of on the agent side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some test conditions are hard to recreate with production backends. VSCode uses both kinds to good effect.
@@ -47,24 +47,19 @@ class CodyAgentService(project: Project) : Disposable { | |||
FixupService.getInstance(project).getSessionForTask(task)?.update(task) | |||
} | |||
|
|||
agent.client.onEditTaskDidDelete = Consumer { task -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you reverted my change to the callbacks/lifecycle managenet.
Was that intentional?
Initialising it in FixupService
won't work in some cases, e.g. after agent restart.
If that was not intentional, please revert those changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveyegge pinging this comment ^
@@ -25,6 +27,71 @@ class FixupService(val project: Project) : Disposable { | |||
// The last text the user typed in without saving it, for continuity. | |||
private var lastPrompt: String = "" | |||
|
|||
init { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert those changes or let's discuss offline.
d6f0473
to
842d42d
Compare
restored a method call that had gone missing
moved the resource files into a testProjects directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be so great to get these going, can't wait to write some.
A few minor comments inline.
This needs rebasing to pick up the changes where 'edit' and 'workspace edit' produce a boolean.
the protocol changes. | ||
|
||
``` | ||
CODY_INTEGRATION_TEST_TOKEN=sgp_asdfasdfasdfasdfasdfasdfasdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a script in sourcegraph/cody, agent/scripts/export-cody-http-recording-tokens.sh . Can we standardize on using those tokens? Everyone has access to them and there are different variants for rate limited tokens, etc. if we need them down the road.
"CODY_JETBRAINS_FEATURES" to "cody.feature.inline-edits=true", | ||
"CODY_RECORDING_MODE" to "replay", | ||
"CODY_RECORDING_DIRECTORY" to "recordings", | ||
"CODY_RECORD_IF_MISSING" to "false") // Polly needs this to record at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...weird!
"CODY_RECORD_IF_MISSING" to "false") // Polly needs this to record at all. | ||
|
||
// Common configuration for integration tests. | ||
// TODO: This doesn't actually work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate?
This PR introduces the new JB integration test framework, based on the built-in JB testing facilities.
Overview
We only have one test so far,
DocumentCodeTest
, but:getFoldingRanges
)This serves as a good reference example of using the test framework.
Note that one facility of the test framework is that we use the built-in JetBrains pubsub support to signal to subscribers (the tests) that certain async checkpoints have been reached, which the tests can await.
FixupSession
and some other classes are instrumented with these notifications, and the tests subscribe to them.Caveats
Test plan
This is a test, so it's part of the test plan for previous PRs.
I have tried not to break anything, so I've verified that on this branch, when using the
stevey/jetbrains-tests' branch of
sourcegraph/cody`:CODY_JETBRAINS_FEATURES
feature flaggradlew intTest
) and from within the IDE