tawada / grass-grower

0 stars 0 forks source link

Refactor mock setup in tests to separate `setup_github` and `setup_llm` fixtures for better modularity, clarity, and efficiency. #105

Open tawada opened 3 weeks ago

tawada commented 3 weeks ago

Issue: Inconsistent Test Setup for GitHub and LLM Services

The existing code contains some inconsistencies and potential improvements regarding the setup of mock functions for GitHub and LLM services in the test cases. These inconsistencies can lead to unreliable or non-reproducible test results.

Observations:

  1. Redundant Calls to Setup Functions:

    • In conftest.py, the setup function calls both setup_github and setup_llm. However, individual setup functions can be invoked directly where needed to avoid redundant setup and teardown steps.

    Current Code:

    @pytest.fixture()
    def setup():
        """Setup mock functions."""
    
        def inner(mocker):
            setup_github()(mocker)
            setup_llm()(mocker)
    
        return inner

    Proposed Change:

      @pytest.fixture()
      def setup_github():
          """Setup mock functions for GitHub."""
          def inner(mocker):
              mocker.patch("services.github.setup_repository", return_value=True)
              mocker.patch("services.github.create_issue", return_value=True)
              mocker.patch("services.github.commit", return_value=True)
              mocker.patch("services.github.push_repository", return_value=True)
              mocker.patch(
                  "services.github.get_issue_by_id",
                  return_value=schemas.Issue(id=1, title="test", body="test", comments=[]),
              )
              mocker.patch("services.github.reply_issue", return_value=True)
              mocker.patch("services.github.checkout_branch", return_value=True)
              mocker.patch("services.github.delete_branch", return_value=True)
          return inner
    
      @pytest.fixture()
      def setup_llm():
          """Setup mock functions for LLM."""
          def inner(mocker):
              mocker.patch.dict("os.environ", {"OPENAI_API_KEY": "test"})
              mocker.patch("services.llm.generate_text", return_value="Hello, world!")
          return inner
  2. Modify Tests to Use Individual Fixtures:

    • Update each test function to directly use the new setup_github and setup_llm fixtures as required.

    Current Example:

    def test_update_issue(mocker, setup):
        """Test update_issue() function."""
        setup(mocker)
    
        mocker.patch("builtins.open", mocker.mock_open(read_data="test"))
        routers.update_issue(1, "test_owner/test_repo", "main", "python")

    Proposed Change:

    def test_update_issue(mocker, setup_github, setup_llm):
       """Test update_issue() function."""
       setup_github(mocker)
       setup_llm(mocker)
       mocker.patch("builtins.open", mocker.mock_open(read_data="test"))
       routers.update_issue(1, "test_owner/test_repo", "main", "python")

Benefits:

Summary:

Refactor the setup of mock functions in the test cases, splitting the setup fixture into two separate fixtures, setup_github and setup_llm, and updating test functions accordingly to directly utilize the necessary fixtures. This will enhance test reliability, clarity, and performance.