Codium-ai / cover-agent

CodiumAI Cover-Agent: An AI-Powered Tool for Automated Test Generation and Code Coverage Enhancement! ๐Ÿ’ป๐Ÿค–๐Ÿงช๐Ÿž
https://www.codium.ai/
GNU Affero General Public License v3.0
3.96k stars 262 forks source link

Golang import issue #74

Closed ProferoAntonKZ closed 3 weeks ago

ProferoAntonKZ commented 4 weeks ago

Hi folks,

There seem to have a problem with how the LLM generates imports for our tests.

It adds it's own import block on top of the package name, in addition to the already existing import block (see screenshot). The code can't compile and the tests wont' pass. It only works for us if all the necessary imports are already there and the llm doesn't have to change them or add anything to them.

How can I explain to the LLM to add imports properly? I tried with the --additional-instructions (which works for other purposes such as function naming conventions) but it didn't understand...

Screenshot 2024-06-02 at 17 00 32

Thanks in advance for any support or suggestion :)

EmbeddedDevops1 commented 4 weeks ago

@mrT23 any ideas on this?

davidparry commented 3 weeks ago

i was also able to reproduce this in the java. Line 375 in the UnitTestGenerator.py is just inserting imports at the top of the file if there are new inserts. I updated my fork and added a more complex example to get the LLM to introduce imports into the test class https://github.com/davidparry/cover-agent/tree/main below show package that needs to be the first line.

import com.davidparry.cover.imp.Fibonacci

package com.davidparry.cover

import spock.lang.Specification

class SimpleMathOperationsSpec extends Specification
mrT23 commented 3 weeks ago

@ProferoAntonKZ thanks for the feedback.

The current logic is to add the imports at the top. At some languages, it really doesn't matter (python). But I see that in Java of Golang for example, the imports need more finesse, for example to come after the package.

I think it's solvable. The same way we ask the model to provide a line number where to introduce new tests, we will ask for a line number where its allowed to introduce new imports.

Can you provide a minimal example that reproduces this in Golang? (hopefully with a minimal installation, as I am not a golang guy) Or maybe I will try with @davidparry Java's example (although I am not a Java guy either :-) )

mrT23 commented 3 weeks ago

Somewhat unrelated, but currently the coverage report analysis is focused on Python. It is very important for the algorithmic flow that we extract the error message from each failed test, and it does not happen in other languages (and also many times we skip that in Python)

@EmbeddedDevops1 - this should be a high-priority goal - to reliably and consistently extract the error messages in other languages, and also to improve this in Python, for all exit codes and scenerios. This is the agent-like behaviour we want.

davidparry commented 3 weeks ago

i have added a simple way to just insert imports which fix this issue in this PR on the second line for imports. I am noticing a problem is new_imports_code that comes back from the LLM is empty and never supplied for java or groovy. If the LLM would give a proper imports_code it gets added properly. @mrT23 i do not object to cancel my PR if you want to use the LLM to find if the import should be added but with the LLM currently not even getting the new_imports_code.

davidparry commented 3 weeks ago

Somewhat unrelated, but currently the coverage report analysis is focused on Python. It is very important for the algorithmic flow that we extract the error message from each failed test, and it does not happen in other languages (and also many times we skip that in Python)

@EmbeddedDevops1 - this should be a high-priority goal - to reliably and consistently extract the error messages in other languages, and also to improve this in Python, for all exit codes and scenerios. This is the agent-like behaviour we want.

I added to the PR the extracting out the failure messages based on @EmbeddedDevops1 feedback. I took into account failure based on test case syntax change and compile failure vs a test case failure.

ProferoAntonKZ commented 3 weeks ago

@ProferoAntonKZ thanks for the feedback.

The current logic is to add the imports at the top. At some languages, it really doesn't matter (python). But I see that in Java of Golang for example, the imports need more finesse, for example to come after the package.

I think it's solvable. The same way we ask the model to provide a line number where to introduce new tests, we will ask for a line number where its allowed to introduce new imports.

Can you provide a minimal example that reproduces this in Golang? (hopefully with a minimal installation, as I am not a golang guy) Or maybe I will try with @davidparry Java's example (although I am not a Java guy either :-) )

Hi @mrT23 , Thanks for looking into this. As I'm working on generating an example to reproduce this, can you perhaps suggest a possible workaround like using --additional-instructions flag to tell the LLM to generate imports in some specific manner?

mrT23 commented 3 weeks ago

@ProferoAntonKZ see this: https://github.com/Codium-ai/cover-agent/pull/82

image image

davidparry commented 3 weeks ago

I merged your branch/PR #82 into the main and tested. The import is working with the relevant_line_number_to_insert_imports_after appears to have a nice consistent suggestion from openAI api. I vote ship it :-) thank you again for updating the prompt and other needs.

davidparry commented 3 weeks ago

@ProferoAntonKZ thanks for the feedback. The current logic is to add the imports at the top. At some languages, it really doesn't matter (python). But I see that in Java of Golang for example, the imports need more finesse, for example to come after the package. I think it's solvable. The same way we ask the model to provide a line number where to introduce new tests, we will ask for a line number where its allowed to introduce new imports. Can you provide a minimal example that reproduces this in Golang? (hopefully with a minimal installation, as I am not a golang guy) Or maybe I will try with @davidparry Java's example (although I am not a Java guy either :-) )

Hi @mrT23 , Thanks for looking into this. As I'm working on generating an example to reproduce this, can you perhaps suggest a possible workaround like using --additional-instructions flag to tell the LLM to generate imports in some specific manner?

I ran the java example and tried little sneaky imports on different lines etc.. the LLM still imported in an acceptable place.