fix(/dev): source folder modification now allows any sub-folder #4489
Conversation
sannicm
force-pushed
the
sannicm/fix/modifySourceFolderWorkspaceCheck
branch
from
11ee25b to
ecbf5c6
Compare
...oftware/aws/toolkits/jetbrains/services/amazonqFeatureDev/controller/FeatureDevController.kt
Outdated
Show resolved
Hide resolved
sannicm
force-pushed
the
sannicm/fix/modifySourceFolderWorkspaceCheck
branch
from
ecbf5c6 to
89d6310
Compare
sannicm
force-pushed
the
sannicm/fix/modifySourceFolderWorkspaceCheck
branch
2 times, most recently
from
36118ff to
9322415
Compare
| @@ -66,9 +66,10 @@ class FeatureDevSessionContext(val project: Project, val maxProjectSizeBytes: Lo | |||
| "dist/" | |||
| ).map { Regex(it) } | |||
|
|
|||
| private var _projectRoot = project.guessProjectDir() ?: error("Cannot guess base directory for project ${project.name}") | |||
| val projectRoot = project.guessProjectDir() ?: error("Cannot guess base directory for project ${project.name}") | |||
| private var _currentRoot = projectRoot | |||
There was a problem hiding this comment.
[question]: I wonder if this is a well-descriptive name or not. Maybe selectedFolder would be better? Since this isn't a root folder anymore.
Also, it would help to have some comments in the code to describe what is projectRoot for and what is currentRoot for.
There was a problem hiding this comment.
I see, yes, it could be made more similar even to what the feature is about, i.e. selectedSourceFolder, and I'll add comments as well, sure.
sannicm
force-pushed
the
sannicm/fix/modifySourceFolderWorkspaceCheck
branch
2 times, most recently
from
585933b to
2054699
Compare
| @@ -390,4 +394,119 @@ class FeatureDevControllerTest : FeatureDevTestBase() { | |||
| newFileContentsCopy[0].rejected = !newFileContentsCopy[0].rejected | |||
| coVerify { messenger.updateFileComponent(testTabId, newFileContentsCopy, deletedFiles, "") } | |||
| } | |||
|
|
|||
| @Test | |||
| fun `test modifyDefaultSourceFolder customer does not select a folder`() = runTest { | |||
There was a problem hiding this comment.
Why are we adding the runTest call here specifically?
There was a problem hiding this comment.
Because the test is verifying and running coroutines and need to be under a scope, it feels cleaner to add the runTest enclosing the whole test.
| whenever(featureDevClient.createTaskAssistConversation()).thenReturn(exampleCreateTaskAssistConversationResponse) | ||
| whenever(chatSessionStorage.getSession(any(), any())).thenReturn(spySession) | ||
|
|
||
| mockkObject(AmazonqTelemetry) |
There was a problem hiding this comment.
We should keep the telemetry portion separate from the implementation, not sure why we are mocking telemetry here?
There was a problem hiding this comment.
The telemetry sends network events, so mocking to me felt right to verify the object's function modifySourceFolder was being called with the expected arguments.
There was a problem hiding this comment.
The telemetry call here is a no-op in the test and anyway telemetry calls are automatically skipped for tests so we needn't mock telemetry here
There was a problem hiding this comment.
I'm removing the verification
sannicm
force-pushed
the
sannicm/fix/modifySourceFolderWorkspaceCheck
branch
3 times, most recently
from
f24e867 to
ebd8954
Compare
sannicm
force-pushed
the
sannicm/fix/modifySourceFolderWorkspaceCheck
branch
from
ebd8954 to
1892fca
Compare
|
Types of changes
Description
The source folder modification happens when the customers repository is too big for uploading and /dev allows the customer to select a new source folder from within their working project. Until now, this check was faulty as it would only check for one level of sub-folder from the root, now it is any level. And of course, folders outside the workspace are not allowed. Workspace being for VSCode the project's root.
Before:
Now:
Checklist
License
I confirm that my contribution is made under the terms of the Apache 2.0 license.