Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Nov 21, 2025

PR Type

Tests


Description

  • Add pytest fixture discovery tests

  • Cover class method target resolution

  • Extend import analysis for fixtures


Diagram Walkthrough

flowchart LR
  tests["tests/test_unit_test_discovery.py"]
  discovery["discover_unit_tests()"]
  imports["analyze_imports_in_test_file()"]
  fixture["pytest fixture handling"]
  target["Class method targets (FunctionParent)"]

  tests -- "new tests" --> discovery
  tests -- "new tests" --> imports
  tests -- "validate" --> fixture
  tests -- "use" --> target
Loading

File Walkthrough

Relevant files
Tests
test_unit_test_discovery.py
Add pytest fixture discovery and import analysis tests     

tests/test_unit_test_discovery.py

  • Import FunctionParent for class context.
  • Add test for pytest fixture-based discovery.
  • Add test for import analysis with fixtures.
  • Verify discovery of class method topologicalSort.
+84/-1   

@aseembits93 aseembits93 requested a review from KRRT7 November 21, 2025 01:32
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Codeflash Bot seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Fixture Signature

The pytest fixtures g are defined with a self parameter, which is invalid for function-scoped fixtures and will cause collection errors. Confirm whether this is intentional; otherwise remove self from the fixture definitions.

@pytest.fixture
def g(self):
    return Graph(6) 
Comment Accuracy

The comment says "Using pytest framework to discover unittest tests" which is misleading since the tests are pure pytest. Consider rephrasing to avoid confusion.

    test_framework="pytest",  # Using pytest framework to discover unittest tests
    tests_project_rootdir=path_obj_tmpdirname.parent,
)
Import Cleanup

Duplicate import of Path appears at the top. Ensure redundant imports are removed to keep tests tidy.

import os
import tempfile
from pathlib import Path

from codeflash.discovery.discover_unit_tests import (
    analyze_imports_in_test_file,
    discover_unit_tests,
    filter_test_files_by_imports,
)
from codeflash.discovery.functions_to_optimize import FunctionToOptimize
from codeflash.models.models import TestsInFile, TestType, FunctionParent
from codeflash.verification.verification_utils import TestConfig


from pathlib import Path

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix invalid pytest fixture signature

Remove the unused self parameter from the pytest fixture; pytest function-scoped
fixtures must not take self unless used within a test class requesting it. Keeping
self will cause a fixture setup error and prevent test discovery/execution.

tests/test_unit_test_discovery.py [744-747]

 @pytest.fixture
-def g(self):
+def g():
     return Graph(6)
Suggestion importance[1-10]: 8

__

Why: The fixture g incorrectly takes self, which will break pytest fixture resolution; removing it matches pytest semantics and ensures tests can run. The existing_code matches the new hunk and the improved_code correctly applies the fix.

Medium
General
Remove trailing whitespace

Normalize trailing whitespace to avoid style check failures in CI environments with
linters enforcing strict whitespace rules. This reduces unnecessary diffs and
potential pre-commit hook rejections.

tests/test_unit_test_discovery.py [744-747]

Suggestion importance[1-10]: 2

__

Why: While trailing whitespace exists after Graph(6), the improved_code is identical to the existing_code and does not remove the whitespace, so the suggestion isn't accurately implemented and has minimal impact.

Low

@aseembits93
Copy link
Contributor Author

@KRRT7 ready to review

Comment on lines 787 to 806
from typing import Any, Dict, List, Optional, Required, TypedDict, Union # noqa: UP035
class LiteLLMParamsTypedDict(TypedDict, total=False):
model: str
custom_llm_provider: Optional[str]
tpm: Optional[int]
rpm: Optional[int]
order: Optional[int]
weight: Optional[int]
max_parallel_requests: Optional[int]
api_key: Optional[str]
api_base: Optional[str]
api_version: Optional[str]
stream_timeout: Optional[Union[float, str]]
max_retries: Optional[int]
organization: Optional[Union[list, str]] # for openai orgs
## DROP PARAMS ##
drop_params: Optional[bool]
## UNIFIED PROJECT/REGION ##
Copy link
Contributor

Choose a reason for hiding this comment

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

if all these are part of the original code and important to it, we should keep it, just skip this test for < 3.10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not super important actually, doesn't impact the actual test

@aseembits93 aseembits93 merged commit 7d18130 into main Nov 21, 2025
21 of 22 checks passed
@aseembits93 aseembits93 deleted the cf-820 branch November 21, 2025 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants