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.

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

feat(amazonq): Adding UTG build and execute logic back for internal Amazon customers. #5396

Open
wants to merge 9 commits into
base: feature/build-execute
Choose a base branch
from

Conversation

Randall-Jiang
Copy link
Contributor

@Randall-Jiang Randall-Jiang commented Feb 20, 2025

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [ X] New feature (non-breaking change which adds functionality)

Description

This PR introduces a build-exec loop for internal Amazon users, allowing them to run a customized build command after Amazon Q generates unit tests. The goal is to ensure that the newly generated unit tests do not break the initial build.

Solution

Adding back build and execute logic for internal amazon users to fix the errors when generating unit tests. Allow users to run 3 iterations to fix the codes if there are any issues cause build failure.
Screenshot 2025-02-19 at 6 25 52 PM

TO-DO

  1. Add telemetry for build & execute loop
  2. Fixing the UI base on the PM's requirement

Checklist

  • [ x] My code follows the code style of this project
  • I have added tests to cover my changes
  • [ x] A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
  • I have added metrics for my changes (if required)

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Randall-Jiang Randall-Jiang requested a review from a team as a code owner February 20, 2025 02:27
@@ -89,6 +88,10 @@
val session = codeTestChatHelper.getActiveSession()
session.isGeneratingTests = true
session.iteration++
if (session.testGenerationJobGroupName.isNullOrEmpty()) {

Check warning

Code scanning / QDJVMC

Useless call on not-null type

Call on not-null type may be reduced
@@ -25,15 +28,19 @@
BuildAndExecuteStatusIcon.WAIT.icon
} else if (progressStatus == BuildAndExecuteProgressStatus.RUN_EXECUTION_TESTS) {
BuildAndExecuteStatusIcon.CURRENT.icon
} else if (progressStatus == BuildAndExecuteProgressStatus.BUILD_FAILED || progressStatus == BuildAndExecuteProgressStatus.FIXING_TEST_CASES) {

Check warning

Code scanning / QDJVMC

Constant conditions

Condition 'progressStatus == BuildAndExecuteProgressStatus.BUILD_FAILED' is always false
val buildLogFilePath = Path.of(utgDir, buildAndExecuteLogDir, "buildAndExecuteLog")
.toString().replace("\\", "/") // Ensures consistent path format

LOG.debug { "Adding build logs to ZIP at: $buildLogFilePath" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this log to make sure the build log is added to the currect folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but we should not print this out for the customers. So, we can remove this.

@Randall-Jiang Randall-Jiang requested a review from a team as a code owner February 20, 2025 18:12

return
}
// taskContext.progressStatus = BuildAndExecuteProgressStatus.RUN_EXECUTION_TESTS
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this commented code? as this is being added as part of this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this commented code is for execution loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a bad practice to leave commented out code as future TODO. Can you put it in a different branch and PR when ready? lets not add clutter

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this code, anyway we will have git history to track.

@@ -821,13 +828,12 @@ class CodeTestChatController(
"Successfully sent test generation telemetry. RequestId: ${
testGenerationEventResponse.responseMetadata().requestId()}"
}
sessionCleanUp(session.tabId)
// sessionCleanUp(session.tabId)
Copy link
Contributor

@laileni-aws laileni-aws Feb 20, 2025

Choose a reason for hiding this comment

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

Remove commented code.

@@ -691,24 +695,27 @@ class CodeTestChatController(
canBeVoted = false
)
)
sessionCleanUp(session.tabId)
if (!isInternalUser) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test this by login with Builder Id vs internal user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@@ -175,7 +179,7 @@ class CodeWhispererUTGChatManager(val project: Project, private val cs: Coroutin
var shortAnswer = ShortAnswer()
LOG.debug {
"Q TestGen session: ${codeTestChatHelper.getActiveCodeTestTabId()}: " +
"polling result for id: ${job.testGenerationJobId()}, group name: ${job.testGenerationJobGroupName()}, " +
"polling result for id: ${job.testGenerationJobId()}, group name: ${session.testGenerationJobGroupName}, " +
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not required as both job.testGenerationJobGroupName() and session.testGenerationJobGroupName will be same

@@ -250,10 +255,9 @@ class CodeWhispererUTGChatManager(val project: Project, private val cs: Coroutin
if (shortAnswer.stopIteration == "true") {
throw CodeTestException("TestGenFailedError: ${shortAnswer.planSummary}", "TestGenFailedError", shortAnswer.planSummary)
}
val fileName = shortAnswer.sourceFilePath?.let { Path.of(it).fileName.toString() } ?: path.fileName.toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we remove this check? Any reason?

codeTestChatHelper.updateAnswer(
CodeTestChatMessageContent(
message = generateSummaryMessage(fileName) + shortAnswer.planSummary,
message = generateSummaryMessage(path.fileName.toString()) + shortAnswer.planSummary,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if user opens test file and run /test. This says Generating unit test for Test file but this should be Source file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when user opens test file and run /test there, if this says Generating unit test for Test file, that's expected right? Because utg is generating unit test for opening files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this code change is from me, I rebased from main and the code change is there. Maybe @ashishrp-aws or @chungjac has more info about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this back?
Looks like it was removed as part of this PR

@@ -297,7 +301,7 @@ class CodeWhispererUTGChatManager(val project: Project, private val cs: Coroutin
// TODO: Modify text according to FnF
codeTestChatHelper.addAnswer(
CodeTestChatMessageContent(
message = message("testgen.error.generic_technical_error_message"),
message = message("testgen.message.failed"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this necessary?
We should show users
I am experiencing technical difficulties at the moment. Please try again … instead of Test generation failed

previousIterationContext?.buildLogFile,
previousIterationContext?.testLogFile
)
val combinedBuildAndExecuteLogFile =
Copy link
Contributor

Choose a reason for hiding this comment

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

variable declaration is not needed here, if required can we change variable name to buildLogFile etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keeped this in case we need excute in the future, will do it in the next pr.

@Randall-Jiang Randall-Jiang force-pushed the build-execute branch 2 times, most recently from bfac8e6 to 24a0832 Compare February 21, 2025 21:50
taskContext.progressStatus = BuildAndExecuteProgressStatus.FIXING_TEST_CASES
val buildAndExecuteMessageId = updateBuildAndExecuteProgressCard(taskContext.progressStatus, messageId, session.iteration)
// TODO: only go to future iterations when buildExitCode or testExitCode > 0, right now iterate regardless
if (taskContext.buildExitCode > 0) {

Check warning

Code scanning / QDJVMC

Constant conditions

Condition 'taskContext.buildExitCode > 0' is always true
requestId = session.startTestGenerationRequestId
)
} else {
AmazonqTelemetry.unitTestGeneration(

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols

'AmazonqTelemetry' is deprecated. Use type-safe metric builders
@@ -114,6 +116,7 @@
private val authController: AuthController = AuthController(),
private val cs: CoroutineScope,
) : InboundAppMessagesHandler {
var buildResult = false

Check notice

Code scanning / QDJVMC

Class member can have 'private' visibility

Property 'buildResult' could be private
requestId = session.startTestGenerationRequestId
)
if (session.iteration == 1) {
AmazonqTelemetry.utgGenerateTests(

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols

'AmazonqTelemetry' is deprecated. Use type-safe metric builders

// Find the nearest Gradle root directory
var packageRoot: File? = testFileAbsolutePath.parentFile
while (packageRoot != null && packageRoot != projectRoot) {

Check warning

Code scanning / QDJVMC

File.equals() usage

Do not use File.equals/hashCode/compareTo as they don't honor case-sensitivity on macOS. Use FileUtil.filesEquals/fileHashCode/compareFiles instead.
@@ -469,10 +473,8 @@ class CodeWhispererUTGChatManager(val project: Project, private val cs: Coroutin
previousIterationContext.selectedFile
}

val combinedBuildAndExecuteLogFile = combineBuildAndExecuteLogFiles(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove combineBuildAndExecuteLogFiles function as this is not being used

val buildLogFilePath = Path.of(utgDir, buildAndExecuteLogDir, "buildAndExecuteLog")
.toString().replace("\\", "/") // Ensures consistent path format

LOG.debug { "Adding build logs to ZIP at: $buildLogFilePath" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but we should not print this out for the customers. So, we can remove this.


return
}
// taskContext.progressStatus = BuildAndExecuteProgressStatus.RUN_EXECUTION_TESTS
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this code, anyway we will have git history to track.

codeTestChatHelper.updateAnswer(
CodeTestChatMessageContent(
message = generateSummaryMessage(fileName) + shortAnswer.planSummary,
message = generateSummaryMessage(path.fileName.toString()) + shortAnswer.planSummary,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this back?
Looks like it was removed as part of this PR

promptInputDisabledState = true,
promptInputProgress = buildAndExecuteProgrogressField,
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can optimize this!

codeTestChatHelper.updateUI(
    promptInputDisabledState = true,
    promptInputProgress = if (session.iteration == 1) {
        testGenProgressField(0)
    } else {
        buildAndExecuteProgrogressField
    }
)

/*
//TODO: this is for unit test regeneration build iteration loop

// TODO: this is for unit test regeneration build iteration loop
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove TODO here

}
val final = session.testGenerationJobGroupName

if (session.iteration == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can optimize this!

codeTestChatHelper.updateUI(
    promptInputDisabledState = true,
    promptInputProgress = if (session.iteration == 1) {
        testGenProgressField(0)
    } else {
        buildAndExecuteProgrogressField
    }
)

// Find the nearest Gradle root directory
var packageRoot: File? = testFileAbsolutePath.parentFile
while (packageRoot != null && packageRoot != projectRoot) {
if (File(packageRoot, "settings.gradle.kts").exists() || File(packageRoot, "build.gradle.kts").exists()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if you're using the groovy version of the gradle build script?


// If no valid Gradle directory is found, fallback to the project root
val workingDir = packageRoot ?: projectRoot
println("Running command in directory: ${workingDir.absolutePath}")
Copy link
Contributor

Choose a reason for hiding this comment

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

use logger instead of print

packageRoot = packageRoot.parentFile
}

// If no valid Gradle directory is found, fallback to the project root
Copy link
Contributor

Choose a reason for hiding this comment

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

? does that actually make sense?

@@ -109,10 +130,14 @@ fun runBuildOrTestCommand(
}

val processBuilder = ProcessBuilder()
.command(listOf(command) + args)
.directory(File(repositoryPath))
.command(listOf("zsh", "-c", "source ~/.zshrc && $command ${args.joinToString(" ")}"))
Copy link
Contributor

Choose a reason for hiding this comment

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

what if they are on windows? or don't use zsh?

Comment on lines +137 to +139
val env = processBuilder.environment()

env["PATH"] = System.getenv("PATH")
Copy link
Contributor

Choose a reason for hiding this comment

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

processes should be started using GeneralCommandLine. see its doc string for an explanation on how the environment variable logic works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants