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

Restrict TestSpark action to suitable code types and fix generation for a line #344

Merged
merged 20 commits into from
Oct 10, 2024

Conversation

arksap2002
Copy link
Collaborator

@arksap2002 arksap2002 commented Sep 3, 2024

Description of changes made

The PR fixes the following issues:

  1. Prohibits action use when non-Java and non-Kotlin file is opened.
  2. Prohibits action use when the cursor is placed on a line that is surrounded by neither method nor class.
  3. Successfully generates tests for a line outside of a method but within a class.
  4. Fixes an issue with line numbering: use 1-based line number invariant for PsiHelper.getSurroundingLineNumber; Kotlin used 0-based numbering which attempted the generated prompt, namely it contained a line before the selected one.

See commit subjects and bodies for details.

What is left

The PR introduces a modification to core module (namely, extends the API of PromptGenerator with an overload of generatePromptForLine method that uses CUT as a line context). Therefore, I (@Vladislav0Art) need to publish a new version of the core module.

TODOs:

  • New core version 4.0.0 is published.

Other notes

Closes #336
Closes #352
Closes #247
Closes #249
Closes #377

  • I have checked that I am merging into the correct branch

@arksap2002 arksap2002 added bug Something isn't working Ready for review PR redy for review labels Sep 3, 2024
@pderakhshanfar
Copy link
Collaborator

It does not solve #352 If I ask to generate test for package line in a kotlin project it throws the following exception:

java.lang.IllegalStateException
	at com.intellij.openapi.progress.util.AbstractProgressIndicatorBase.throwInvalidState(AbstractProgressIndicatorBase.java:130)
	at com.intellij.openapi.progress.util.AbstractProgressIndicatorBase.stop(AbstractProgressIndicatorBase.java:121)
	at com.intellij.openapi.progress.util.AbstractProgressIndicatorExBase.stop(AbstractProgressIndicatorExBase.java:52)
	at com.intellij.openapi.progress.util.AbstractProgressIndicatorExBase.delegate(AbstractProgressIndicatorExBase.java:211)
	at com.intellij.openapi.progress.util.AbstractProgressIndicatorExBase.doDelegateRunningChange(AbstractProgressIndicatorExBase.java:195)
	at com.intellij.openapi.progress.util.AbstractProgressIndicatorExBase.stop(AbstractProgressIndicatorExBase.java:53)
	at com.intellij.openapi.wm.impl.status.InfoAndProgressPanel$MyInlineProgressIndicator.stop(InfoAndProgressPanel.kt:830)
	at com.intellij.openapi.progress.util.AbstractProgressIndicatorExBase.delegate(AbstractProgressIndicatorExBase.java:211)
	at com.intellij.openapi.progress.util.AbstractProgressIndicatorExBase.doDelegateRunningChange(AbstractProgressIndicatorExBase.java:195)
	at com.intellij.openapi.progress.util.AbstractProgressIndicatorExBase.stop(AbstractProgressIndicatorExBase.java:53)
	at com.intellij.openapi.progress.util.ProgressWindow.stop(ProgressWindow.java:285)
	at org.jetbrains.research.testspark.display.custom.IJProgressIndicator.stop(IJProgressIndicator.kt:32)
	at org.jetbrains.research.testspark.tools.ToolUtils.isProcessCanceled(ToolUtils.kt:117)
	at org.jetbrains.research.testspark.tools.ToolUtils.isProcessStopped(ToolUtils.kt:112)
	at org.jetbrains.research.testspark.tools.Pipeline$runTestGeneration$1.run(Pipeline.kt:107)
	at com.intellij.openapi.progress.impl.CoreProgressManager.startTask(CoreProgressManager.java:477)
	at com.intellij.openapi.progress.impl.ProgressManagerImpl.startTask(ProgressManagerImpl.java:133)
	at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$runProcessWithProgressAsynchronously$6(CoreProgressManager.java:528)
	at com.intellij.openapi.progress.impl.ProgressRunner.lambda$submit$4(ProgressRunner.java:250)
	at com.intellij.openapi.progress.ProgressManager.lambda$runProcess$0(ProgressManager.java:100)
	at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$runProcess$1(CoreProgressManager.java:221)
	at com.intellij.platform.diagnostic.telemetry.helpers.TraceKt.use(trace.kt:46)
	at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$runProcess$2(CoreProgressManager.java:220)
	at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$executeProcessUnderProgress$13(CoreProgressManager.java:660)
	at com.intellij.openapi.progress.impl.CoreProgressManager.registerIndicatorAndRun(CoreProgressManager.java:735)
	at com.intellij.openapi.progress.impl.CoreProgressManager.computeUnderProgress(CoreProgressManager.java:691)
	at com.intellij.openapi.progress.impl.CoreProgressManager.executeProcessUnderProgress(CoreProgressManager.java:659)
	at com.intellij.openapi.progress.impl.ProgressManagerImpl.executeProcessUnderProgress(ProgressManagerImpl.java:79)
	at com.intellij.openapi.progress.impl.CoreProgressManager.runProcess(CoreProgressManager.java:202)
	at com.intellij.openapi.progress.ProgressManager.runProcess(ProgressManager.java:100)
	at com.intellij.openapi.progress.impl.ProgressRunner.lambda$submit$5(ProgressRunner.java:250)
	at com.intellij.openapi.progress.impl.ProgressRunner$ProgressRunnable.run(ProgressRunner.java:500)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.util.concurrent.Executors$PrivilegedThreadFactory$1$1.run(Executors.java:702)
	at java.base/java.util.concurrent.Executors$PrivilegedThreadFactory$1$1.run(Executors.java:699)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.util.concurrent.Executors$PrivilegedThreadFactory$1.run(Executors.java:699)
	at java.base/java.lang.Thread.run(Thread.java:840)

@pderakhshanfar pderakhshanfar added the In progress PR is in progress label Sep 17, 2024
@Vladislav0Art
Copy link
Collaborator

Vladislav0Art commented Sep 30, 2024

As @pderakhshanfar metioned above, if you attempt generation on an import you'l' get an exception. In my case, it was:

java.lang.NullPointerException
	at org.jetbrains.research.testspark.tools.llm.generation.PromptManager.getMethodDescriptor(PromptManager.kt:294)
	at org.jetbrains.research.testspark.tools.llm.generation.PromptManager.generatePrompt$lambda$6(PromptManager.kt:128)
	at com.intellij.openapi.application.impl.RwLockHolder.runReadAction(RwLockHolder.kt:271)
	at com.intellij.openapi.application.impl.ApplicationImpl.runReadAction(ApplicationImpl.java:845)
	at org.jetbrains.research.testspark.tools.llm.generation.PromptManager.generatePrompt(PromptManager.kt:69)
	at org.jetbrains.research.testspark.tools.llm.generation.LLMProcessManager.runTestGenerator(LLMProcessManager.kt:105)
	at org.jetbrains.research.testspark.tools.Pipeline$runTestGeneration$1.run(Pipeline.kt:96)
	at com.intellij.openapi.progress.impl.CoreProgressManager.startTask(CoreProgressManager.java:477)
	at com.intellij.openapi.progress.impl.ProgressManagerImpl.startTask(ProgressManagerImpl.java:133)
	at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$runProcessWithProgressAsynchronously$6(CoreProgressManager.java:528)
	at com.intellij.openapi.progress.impl.ProgressRunner.lambda$submit$4(ProgressRunner.java:250)
	at com.intellij.openapi.progress.ProgressManager.lambda$runProcess$0(ProgressManager.java:100)
	at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$runProcess$1(CoreProgressManager.java:221)
	at com.intellij.platform.diagnostic.telemetry.helpers.TraceKt.use(trace.kt:46)
	at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$runProcess$2(CoreProgressManager.java:220)
	at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$executeProcessUnderProgress$13(CoreProgressManager.java:660)
	at com.intellij.openapi.progress.impl.CoreProgressManager.registerIndicatorAndRun(CoreProgressManager.java:735)
	at com.intellij.openapi.progress.impl.CoreProgressManager.computeUnderProgress(CoreProgressManager.java:691)
	at com.intellij.openapi.progress.impl.CoreProgressManager.executeProcessUnderProgress(CoreProgressManager.java:659)
	at com.intellij.openapi.progress.impl.ProgressManagerImpl.executeProcessUnderProgress(ProgressManagerImpl.java:79)
	at com.intellij.openapi.progress.impl.CoreProgressManager.runProcess(CoreProgressManager.java:202)
	at com.intellij.openapi.progress.ProgressManager.runProcess(ProgressManager.java:100)
	at com.intellij.openapi.progress.impl.ProgressRunner.lambda$submit$5(ProgressRunner.java:250)
	at com.intellij.openapi.progress.impl.ProgressRunner$ProgressRunnable.run(ProgressRunner.java:500)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.util.concurrent.Executors$PrivilegedThreadFactory$1$1.run(Executors.java:702)
	at java.base/java.util.concurrent.Executors$PrivilegedThreadFactory$1$1.run(Executors.java:699)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.util.concurrent.Executors$PrivilegedThreadFactory$1.run(Executors.java:699)
	at java.base/java.lang.Thread.run(Thread.java:840)
2024-09-30 17:18:58,507 [  38600] SEVERE - #c.i.o.p.Task - IntelliJ IDEA 2024.1  Build #IC-241.14494.240
2024-09-30 17:18:58,507 [  38600] SEVERE - #c.i.o.p.Task - JDK: 17.0.10; VM: OpenJDK 64-Bit Server VM; Vendor: JetBrains s.r.o.
2024-09-30 17:18:58,507 [  38600] SEVERE - #c.i.o.p.Task - OS: Mac OS X
2024-09-30 17:18:58,508 [  38601] SEVERE - #c.i.o.p.Task - Plugin to blame: TestSpark version: 0.2.1
2024-09-30 17:18:58,508 [  38601] SEVERE - #c.i.o.p.Task - Last Action: TestSpark.TestSparkActions
2024-09-30 17:18:58,510 [  38603] SEVERE - #c.i.o.a.i.FlushQueue - null

It is because we assert that a surrounding method exists. In general, we should quit using !! in TestSpark. It has always dealt us a bad hand:

Screenshot 2024-09-30 at 17 22 29

We should hide the action if there is no valid code construct under test. For a given line, we need to ensure it is within a method, class, or top-level function. The check should be performed in this order: 1) search for a surrounding method; if not found, 2) check for a surrounding class; if not found, 3) check for a top-level function. If none exist, the line is top-level, and we cannot generate tests, so the action should not appear in the right-click menu.

@Vladislav0Art
Copy link
Collaborator

The same error as here occurs if I start generation for a line (**) and select line as a code construct under test:

public class Calc {
    int a = 10; // (**)

    public int sum(int a, int b) {
        return a + b;
    }

    public int multiply(int a, int b) {
        return a * b;
    }
}

Copy link
Collaborator

@Vladislav0Art Vladislav0Art left a comment

Choose a reason for hiding this comment

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

There are important comments above that identify unfixed bugs. Those should be addressed in this PR to close the issue #352.

@Vladislav0Art Vladislav0Art removed the Ready for review PR redy for review label Oct 4, 2024
@Vladislav0Art Vladislav0Art self-assigned this Oct 4, 2024
@Vladislav0Art Vladislav0Art force-pushed the arksap2002/bugs/fix-anaction-update branch from b40a077 to 3079173 Compare October 4, 2024 14:39
@Vladislav0Art
Copy link
Collaborator

It does not solve #352 If I ask to generate test for package line in a kotlin project it throws the following exception:

Fixed.

@Vladislav0Art Vladislav0Art changed the title Fix TestSparkAction/update function Restrict TestSpark action to suitable code types and fix generation for a line Oct 7, 2024
@Vladislav0Art Vladislav0Art mentioned this pull request Oct 7, 2024
1 task
@Vladislav0Art
Copy link
Collaborator

The same error as here occurs if I start generation for a line (**) and select line as a code construct under test:

Fixed.

@Vladislav0Art
Copy link
Collaborator

As @pderakhshanfar metioned above, if you attempt generation on an import you'l' get an exception. In my case, it was:

Fixed.

@Vladislav0Art Vladislav0Art self-requested a review October 7, 2024 12:50
Copy link
Collaborator

@Vladislav0Art Vladislav0Art left a comment

Choose a reason for hiding this comment

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

Introduced modifications that solve the described problems.

@Vladislav0Art Vladislav0Art self-requested a review October 7, 2024 12:50
Copy link
Collaborator

@Vladislav0Art Vladislav0Art left a comment

Choose a reason for hiding this comment

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

I must approve the PR to allow its further merge. This approval does not count.

@Vladislav0Art Vladislav0Art added Ready for review PR redy for review and removed In progress PR is in progress labels Oct 7, 2024
@Vladislav0Art Vladislav0Art added the Urgent Urgant PR label Oct 7, 2024
@Hello-zoka
Copy link
Collaborator

Hello-zoka commented Oct 7, 2024

I've found several problems connected with line generation:

  1. It doesn't include information about the class line refers to. For example in method generation we have smth like: Generate unit tests in kotlin for org.example.Calc.mul to achieve 100% line coverage for this method.
    For example, when asked to generate a test for the line 'return a + b' in the following code:
class Calc {
    fun sum(a: Int, b: Int): Int {
        return a + b
    }
 }

LLM is struggling with generating Calc().sum() calls and spending attempts on generating uncompilable sum() calls. This leads to unnecessary feedback cycles iterations.

This problem became even more crucial if attempted to generate a test for a line inside a companion object, or it's method. LLM just don't know anything about how to call such function.

Solution: includes information about the class surrounding the line if such is present.

  1. We are not allowing line generation for empty lines, but it can be asked for comment lines if it is surrounded by class/function. The result will be almost the same as for the whole class test generation, but it's weird

  2. Adding to the previous point, there is a strange restriction for such a top-level function:

/// 123 -- not allowed for line generation

// 123 -- allowed for line generation
fun multwice(a: Int) = a * 2

Solution idea: For each line generation, check if it includes something besides comments, open/closed brackets and allow generation only in such case

@Vladislav0Art
Copy link
Collaborator

I've found several problems connected with line generation:

  1. It doesn't include information about the class line refers to. For example in method generation we have smth like: Generate unit tests in kotlin for org.example.Calc.mul to achieve 100% line coverage for this method.
    For example, when asked to generate a test for the line 'return a + b' in the following code:
class Calc {
    fun sum(a: Int, b: Int): Int {
        return a + b
    }
 }

LLM is struggling with generating Calc().sum() calls and spending attempts on generating uncompilable sum() calls. This leads to unnecessary feedback cycles iterations.

This problem became even more crucial if attempted to generate a test for a line inside a companion object, or it's method. LLM just don't know anything about how to call such function.

Solution: includes information about the class surrounding the line if such is present.

  1. We are not allowing line generation for empty lines, but it can be asked for comment lines if it is surrounded by class/function. The result will be almost the same as for the whole class test generation, but it's weird
  2. Adding to the previous point, there is a strange restriction for such a top-level function:
/// 123 -- not allowed for line generation

// 123 -- allowed for line generation
fun multwice(a: Int) = a * 2

Solution idea: For each line generation, check if it includes something besides comments, open/closed brackets and allow generation only in such case

@Hello-zoka, I agree with the 2nd and 3rd points; I'll address them in the PR or explain why they should be addressed in a different PR.

At first glance, the first point does not appear to be an easy task, and it does not represent a bug; it is a lack of context. It is a stand-alone issue that should be addressed in a different PR (possibly after the upcoming release). @Hello-zoka, could you create an issue for it?

My main focus, for now, is to close as many issues as possible that yield runtime exceptions and fail the plugin. The aspects regarding line-based test generation mentioned appear minor to me as very few people use line-based test generation compared to method/class-based generation (a good thing to calculate it as a metric! @arksap2002).

CC: @pderakhshanfar

@Vladislav0Art
Copy link
Collaborator

Vladislav0Art commented Oct 8, 2024

I've found several problems connected with line generation:

  1. It doesn't include information about the class line refers to. For example in method generation we have smth like: Generate unit tests in kotlin for org.example.Calc.mul to achieve 100% line coverage for this method.
    For example, when asked to generate a test for the line 'return a + b' in the following code:
class Calc {
    fun sum(a: Int, b: Int): Int {
        return a + b
    }
 }

LLM is struggling with generating Calc().sum() calls and spending attempts on generating uncompilable sum() calls. This leads to unnecessary feedback cycles iterations.

This problem became even more crucial if attempted to generate a test for a line inside a companion object, or it's method. LLM just don't know anything about how to call such function.

Solution: includes information about the class surrounding the line if such is present.

  1. We are not allowing line generation for empty lines, but it can be asked for comment lines if it is surrounded by class/function. The result will be almost the same as for the whole class test generation, but it's weird
  2. Adding to the previous point, there is a strange restriction for such a top-level function:
/// 123 -- not allowed for line generation

// 123 -- allowed for line generation
fun multwice(a: Int) = a * 2

Solution idea: For each line generation, check if it includes something besides comments, open/closed brackets and allow generation only in such case

@Hello-zoka
Regarding points 2 and 3, I will create a separate issue for them. Both require modifications in how we select PSI elements in PsiHelper.getCurrentListOfCodeTypes + it needs additional parsing, which is out of the scope of the current PR.

Reflected in #377, #378.

Copy link
Collaborator

@Hello-zoka Hello-zoka left a comment

Choose a reason for hiding this comment

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

It seems like it is easy to fix #377 by refactoring how you generate the prompt for the line. In my opinion, it has to be in this PR since it is about "fix generation for the line" + #337 depends on the changes from this PR.

@Vladislav0Art Vladislav0Art added the changes requested Reviewer(s) left some comments that should be addressed by PR maintainer label Oct 8, 2024
@Vladislav0Art
Copy link
Collaborator

Here is the comment of PromptGenerator.buildCutDeclaration method (so that you do not need to fetch the branch):

Screenshot 2024-10-09 at 18 49 52

@Vladislav0Art Vladislav0Art added Ready for review PR redy for review and removed changes requested Reviewer(s) left some comments that should be addressed by PR maintainer labels Oct 9, 2024
Copy link
Collaborator

@pderakhshanfar pderakhshanfar left a comment

Choose a reason for hiding this comment

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

Nit (and we dont need to resolve it in this issue) TestSpark option is still available when we try to generate test for a test file:
open a test file right click and ask for test generation using TestSpark

arksap2002 and others added 19 commits October 10, 2024 12:25
The bug is reflected in the issue #375.
…umbers

Before, the `KotlinPsiHelper` returned a 0-based line number which caused an issue with line-based test generation.
The generated prompt contained a line above the selected one.
When there is no surrounding method about the selected line,
we use the CUT as a context for this line. The CUT must always be present.
Otherwise, the generation action should have been disabled for this line.
The line-based test generation that has a method as a context
of the line now also accepts constructors of the containing class.
@Vladislav0Art Vladislav0Art force-pushed the arksap2002/bugs/fix-anaction-update branch from eb19a43 to 7d99e58 Compare October 10, 2024 10:30
The major version increased due to the change of the public API of `PromptGenerator.generatePromptForLine` method.
@Vladislav0Art Vladislav0Art merged commit d696259 into development Oct 10, 2024
3 checks passed
@arksap2002 arksap2002 deleted the arksap2002/bugs/fix-anaction-update branch October 15, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Ready for review PR redy for review Urgent Urgant PR
Projects
None yet
4 participants