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

Fixed extra prompt build caller. #249

Conversation

EmbeddedDevops1
Copy link
Collaborator

@EmbeddedDevops1 EmbeddedDevops1 commented Dec 17, 2024

PR Type

Bug fix


Description

  • Removed unnecessary call to test_gen.build_prompt() in the CoverAgent.init() method
  • The prompt building is already handled properly in the run_test_gen() method where its result is actually used
  • This change eliminates redundant processing and potential side effects
  • Updated version number to reflect the bug fix

Changes walkthrough 📝

Relevant files
Bug fix
CoverAgent.py
Remove redundant prompt building call in initialization   

cover_agent/CoverAgent.py

  • Removed redundant call to test_gen.build_prompt() in init() method
  • The removed call was unnecessary as the prompt is built later in
    run_test_gen()
  • +0/-1     
    Configuration changes
    version.txt
    Version bump to 0.2.10                                                                     

    cover_agent/version.txt

    • Bumped version from 0.2.9 to 0.2.10
    +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @EmbeddedDevops1
    Copy link
    Collaborator Author

    ⌛ Running regression test suite: https://github.com/qodo-ai/qodo-cover/actions/runs/12379255839

    @EmbeddedDevops1
    Copy link
    Collaborator Author

    Regression test alone is not nearly enough to approve an algo change.

    Have you actually run this code in various scenarios, catched a breakpoint, and validated the indeed this line is not needed, and no new variables are initialized there ? (hint - it is needed)

    I ran through a few test projects using the regression script. Here's why I'm thinking it's not needed. The function we're calling doesn't actually set anything at the class level:

        def build_prompt(self, failed_test_runs, language, testing_framework, code_coverage_report) -> dict:
            """
            Builds a prompt using the provided information to be used for generating tests.
    
            This method checks for the existence of failed test runs and then calls the PromptBuilder class to construct the prompt.
            The prompt includes details such as the source file path, test file path, code coverage report, included files,
            additional instructions, failed test runs, and the programming language being used.
    
            Returns:
                str: The generated prompt to be used for test generation.
            """
            # Check for existence of failed tests:
            if not failed_test_runs:
                failed_test_runs_value = ""
            else:
                failed_test_runs_value = ""
                try:
                    for failed_test in failed_test_runs:
                        failed_test_dict = failed_test.get("code", {})
                        if not failed_test_dict:
                            continue
                        # dump dict to str
                        code = json.dumps(failed_test_dict)
                        error_message = failed_test.get("error_message", None)
                        failed_test_runs_value += f"Failed Test:\n```\n{code}\n```\n"
                        if error_message:
                            failed_test_runs_value += (
                                f"Test execution error analysis:\n{error_message}\n\n\n"
                            )
                        else:
                            failed_test_runs_value += "\n\n"
                except Exception as e:
                    self.logger.error(f"Error processing failed test runs: {e}")
                    failed_test_runs_value = ""
    
            # Call PromptBuilder to build the prompt
            self.prompt_builder = PromptBuilder(
                source_file_path=self.source_file_path,
                test_file_path=self.test_file_path,
                code_coverage_report=code_coverage_report,
                included_files=self.included_files,
                additional_instructions=self.additional_instructions,
                failed_test_runs=failed_test_runs_value,
                language=language,
                testing_framework=testing_framework,
                project_root=self.project_root,
            )
    
            return self.prompt_builder.build_prompt()

    It returns a dict but we never actually store that into a variable:

    self.test_gen.build_prompt(failed_test_runs, language, test_framework, coverage_report)
    

    So what purpose does this serve? Are we just testing that the setup is good and no assertion failures are occurring? If that's the case then I would revert my last commit but I would make sure to add a comment stating exactly that in this MR.

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Dec 17, 2024

    The main mistake was to merge the "Refactor" to the algo without my approval.
    The order now just doesn't make sense, and is extremely error prone

    both "test_validator" and "test_gen" contain different prompt object,
    The "test_validator" object has outdated prompt (without 'test_framework' for example). If its used anywhere, its abug

    algo flow should be clear and coherent, and doesn't need wrapping of these complicated classes that prevent from understanding what is actually going on

    image

    @EmbeddedDevops1 EmbeddedDevops1 merged commit 356a142 into main Dec 17, 2024
    8 of 9 checks passed
    @EmbeddedDevops1 EmbeddedDevops1 deleted the 245-redundant-call-of-selftest_genbuild_promptfailed_test_runs-language-test_framework-coverage_report-inside-of-coveragentinit branch December 17, 2024 19:49
    @mrT23
    Copy link
    Collaborator

    mrT23 commented Dec 17, 2024

    (i deleted my old comment because after reviewing the complicated flow, I understood it was wrong, and the change here is justified.

    But the main issue remains - the current structure is too complicated, and the flow is not clear enough. too many
    self.init() ... which are just too abstract and not clear. Flow should be clear without going to each function and understanding every detail in it)

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Dec 17, 2024

    /describe

    Copy link
    Contributor

    PR Description updated to latest commit (6684306)

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