feat: Add pytest plugin with result caching for unchanged scenarios - #46
Conversation
…viralgarg05#26) - Add pytest plugin for AgentUnit scenario discovery and execution - Add ScenarioCache class to hash scenario inputs and store results - Store cache in .agentunit_cache/ directory with auto .gitignore - Add --no-cache flag to bypass cache and force fresh runs - Add --clear-cache flag to clear cache before running tests - Invalidate cache when source files change - Add comprehensive tests for plugin and caching functionality - Add documentation for pytest plugin usage
Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
WalkthroughAdds a file-based ScenarioCache and CLI init command, integrates caching and cache-related pytest options into the AgentUnit pytest plugin, supplies example scenarios and documentation updates, and adds tests for the new cache behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Pytest
participant Plugin as AgentUnit Plugin
participant Collector as AgentUnitFile
participant Item as AgentUnitItem
participant Cache as ScenarioCache
participant Runner as run_suite
Pytest->>Plugin: pytest_configure (reads flags)
Plugin->>Cache: Initialize ScenarioCache (enabled/disabled)
Plugin->>Cache: clear() if --clear-cache
Pytest->>Plugin: pytest_collect_file (tests/eval/*.py or configs)
Plugin->>Collector: create AgentUnitFile
Collector->>Collector: discover scenarios (python factories or config)
Collector->>Item: create AgentUnitItem per scenario (attach source_path)
Pytest->>Item: runtest()
Item->>Cache: get(scenario, source_path)
alt Cache hit (valid)
Cache-->>Item: CachedResult(success=true/false)
Item->>Pytest: report cached result (skip execution or fail early)
else Cache miss or invalid
Item->>Runner: run_suite(scenario)
Runner-->>Item: success / failures
Item->>Cache: set(scenario, success, failures, source_path)
Item->>Pytest: report runtime result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
docs/pytest-plugin.md (2)
24-29: Minor: Clarify "nocode module" reference.The term "nocode" may be unfamiliar to users. Consider adding a brief explanation or link to the nocode module documentation if it exists.
177-190: Add language specifier to the fenced code block.The directory structure block on line 179 is flagged by markdownlint for missing a language specifier. Use
textorplaintextfor non-code blocks.🔎 Proposed fix
-``` +```text project/ ├── tests/src/agentunit/pytest/cache.py (2)
70-85: Broad exception handling could hide bugs.The bare
except Exceptionon line 83 silently catches all errors when iterating dataset cases, including programming errors like AttributeError or TypeError. This could mask bugs in dataset implementations.🔎 Suggested fix: Log exceptions for debugging
# Hash dataset cases try: cases = list(scenario.dataset.iter_cases()) hash_data["dataset"] = { "name": scenario.dataset.name, "cases": [ { "id": case.id, "query": case.query, "expected_output": case.expected_output, } for case in cases ], } - except Exception: + except Exception as e: # If we can't iterate cases, use dataset name only + logger.debug(f"Failed to iterate dataset cases for {scenario.name}: {e}") hash_data["dataset"] = {"name": scenario.dataset.name}
147-179: Consider consistent return value for disabled cache.When the cache is disabled, the method returns an empty string (line 156), but when enabled, it returns the cache key (line 178). This inconsistency could confuse callers who might not expect different return types.
🔎 Suggested fix: Return cache key even when disabled
def set( self, scenario: Scenario, success: bool, failures: list[str], source_path: Path | None = None, ) -> str: """Store scenario result in cache.""" + cache_key = self._compute_scenario_hash(scenario) if not self.enabled: - return "" + return cache_key self._ensure_cache_dir() - cache_key = self._compute_scenario_hash(scenario) source_hash = self._compute_source_hash(source_path)This maintains a consistent return value while still skipping cache writes when disabled.
src/agentunit/pytest/plugin.py (3)
120-147: Consider logging skipped scenario factories for debugging.Lines 136-144 attempt to call functions prefixed with
scenario_but silently skip those that fail (e.g., functions requiring arguments). While this behavior prevents crashes, it could hide configuration issues. Adding debug logging would help developers identify why their scenario factories aren't discovered.🔎 Suggested enhancement: Add debug logging
elif callable(obj) and name.startswith("scenario_"): # Try to call functions that look like scenario factories try: result = obj() if isinstance(result, Scenario): scenarios.append(result) - except Exception: + else: + logger.debug(f"Function {name} didn't return Scenario: {type(result)}") + except Exception as e: # Skip functions that can't be called or don't return scenarios + logger.debug(f"Failed to call scenario factory {name}: {e}") continueNote on duplicate discovery: Line 132 uses
dir(module), which includes imported names. If multiple test files import the same scenario, it will be discovered multiple times. This might be intentional for flexibility, but be aware scenarios imported viafrom other_module import my_scenariowill appear in each importing module's test collection.
240-240: Minor: Simplify success check.Line 240 uses
len(failures) == 0which can be simplified tonot failuresfor better Python idiomaticity.- success = len(failures) == 0 + success = not failures
46-67: Consider using pytest's stash API for plugin data storage instead of private attributes.Storing the cache using a private attribute (
config._agentunit_cacheat line 61) works but violates encapsulation and could conflict with other plugins. Pytest offers the stash API as a better alternative for plugin-specific storage.If your project supports pytest 7.0+, refactor to use the stash API: Create a StashKey at the module level, then store/retrieve the cache using
config.stashinstead of the private attribute. This requires updating line 61 and the_get_cache()method at line 201-203. First verify your minimum pytest version requirement supports this change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/pytest-plugin.md(1 hunks)src/agentunit/pytest/__init__.py(1 hunks)src/agentunit/pytest/cache.py(1 hunks)src/agentunit/pytest/cli.py(1 hunks)src/agentunit/pytest/plugin.py(1 hunks)tests/eval/__init__.py(1 hunks)tests/eval/example_scenarios.py(1 hunks)tests/test_pytest_plugin.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/agentunit/pytest/cache.py (2)
src/agentunit/core/scenario.py (1)
Scenario(23-278)src/agentunit/datasets/base.py (1)
name(38-39)
src/agentunit/pytest/__init__.py (1)
src/agentunit/pytest/plugin.py (3)
pytest_addoption(27-43)pytest_collect_file(69-79)pytest_configure(46-66)
tests/eval/example_scenarios.py (3)
src/agentunit/core/scenario.py (1)
Scenario(23-278)src/agentunit/adapters/base.py (2)
AdapterOutcome(18-25)BaseAdapter(28-69)src/agentunit/datasets/base.py (3)
DatasetCase(19-27)DatasetSource(30-57)name(38-39)
src/agentunit/pytest/plugin.py (5)
src/agentunit/core/runner.py (1)
run_suite(101-111)src/agentunit/core/exceptions.py (1)
AgentUnitError(8-11)src/agentunit/pytest/cache.py (3)
ScenarioCache(32-194)clear(180-194)get(106-145)src/agentunit/datasets/base.py (1)
name(38-39)src/agentunit/nocode/builder.py (1)
ScenarioBuilder(21-398)
🪛 LanguageTool
docs/pytest-plugin.md
[grammar] ~29-~29: Ensure spelling is correct
Context: ...l, .json`): Loads scenarios using the nocode module ### Python Scenario Files Crea...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/pytest-plugin.md
179-179: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (35)
tests/eval/__init__.py (1)
1-1: LGTM!Minimal package marker file that aligns with the CLI-generated content in
cli.py.tests/eval/example_scenarios.py (6)
1-6: LGTM!Clean imports aligned with the AgentUnit API surface.
8-26: LGTM!Good implementation of
BaseAdapterwith proper error handling inexecute. The exception handling prevents unhandled errors from propagating and instead returns a structuredAdapterOutcomewith the error message.
29-49: LGTM!Proper use of
DatasetSourcewith a method reference as the loader callback.
52-61: LGTM!Simple and clear example agent implementation demonstrating the expected input/output contract.
64-69: LGTM!Module-level
Scenarioinstance for auto-discovery by the pytest plugin.
72-105: LGTM!Good demonstration of the factory function pattern with encapsulated dataset and agent definitions. The
scenario_*naming convention enables auto-discovery.src/agentunit/pytest/__init__.py (1)
1-5: LGTM!Clean module surface that re-exports the essential pytest plugin hooks, enabling users to register the plugin via entry points or direct import.
docs/pytest-plugin.md (1)
1-11: LGTM!Clear introduction and installation instructions.
src/agentunit/pytest/cli.py (3)
1-8: LGTM!Standard imports for a click-based CLI module.
46-191: LGTM!The embedded content approach is appropriate for generating example scaffolding. The content correctly mirrors
tests/eval/example_scenarios.py.
194-195: LGTM!Standard
__main__guard for script invocation.tests/test_pytest_plugin.py (15)
1-10: LGTM!Appropriate imports for testing the plugin components.
12-31: LGTM!Test-specific adapter implementation. The duplication from
example_scenarios.pyis acceptable for test isolation.
33-57: LGTM!Minimal mock classes that provide sufficient pytest infrastructure for unit testing the plugin components.
59-69: LGTM!Good coverage of the
_is_eval_directoryhelper with both positive and negative test cases, including edge cases likeeval/scenarios.py(missingtests/prefix).
70-121: LGTM!Comprehensive test for scenario discovery from Python files. Uses
tmp_pathfixture appropriately and verifies the discovery of module-levelScenarioinstances.
122-147: LGTM!Tests the success path through
AgentUnitItem.runtest().
149-176: LGTM!Tests the failure path with proper assertion message verification.
178-186: LGTM!Tests the load error path, verifying that
AgentUnitErroris raised with the expected message.
188-217: LGTM!Verifies that pytest markers (
agentunitandscenario) are properly attached to test items.
220-256: LGTM!Good test coverage for the basic cache lifecycle: creation, storage, and retrieval.
258-285: LGTM!Properly tests that cache operations become no-ops when
enabled=False, returning empty string forset()andNoneforget().
287-323: LGTM!Critical test for cache invalidation when source files change. Verifies that modifying
source_pathcontent causesget()to returnNone.
325-355: LGTM!Tests the
clear()operation and verifies that cached entries are removed.
357-388: LGTM!Tests that failure information is correctly persisted and retrieved from the cache.
390-442: LGTM!Important test verifying that different scenarios (with different datasets/names) produce distinct cache keys, preventing cache collisions.
src/agentunit/pytest/cache.py (4)
1-48: LGTM! Clean initialization and directory setup.The module structure, constants, and cache directory initialization are well-implemented. The automatic .gitignore creation prevents accidental cache commits.
91-105: LGTM! Source hash computation handles edge cases well.The optional source hash for cache invalidation is implemented correctly, with appropriate None-returns for missing files and errors.
106-146: LGTM! Cache retrieval with proper invalidation logic.The
getmethod correctly validates cached results against source file changes and handles errors gracefully with appropriate logging.
180-194: LGTM! Cache clearing with resilient error handling.The
clearmethod safely removes cache files with best-effort error handling, which is appropriate for cleanup operations.src/agentunit/pytest/plugin.py (4)
1-44: LGTM! Clean plugin setup with clear command-line options.The pytest option registration is well-structured with descriptive help text and appropriate defaults.
89-119: LGTM! Robust scenario collection with error handling.The collection logic properly handles discovery failures by creating a load_error test item, ensuring problems are visible in test results rather than silently skipped.
179-204: LGTM! Clean test item initialization with proper markers.The AgentUnitItem correctly integrates with pytest's marker system and safely retrieves the cache instance.
256-264: LGTM! Clean test reporting implementation.The failure representation and test location reporting are well-structured and provide clear information for pytest output.
| def _compute_scenario_hash(self, scenario: Scenario) -> str: | ||
| """Compute hash from scenario inputs (dataset, adapter config).""" | ||
| hash_data: dict[str, Any] = { | ||
| "version": CACHE_VERSION, | ||
| "name": scenario.name, | ||
| "retries": scenario.retries, | ||
| "max_turns": scenario.max_turns, | ||
| "timeout": scenario.timeout, | ||
| "tags": scenario.tags, | ||
| "seed": scenario.seed, | ||
| "metadata": scenario.metadata, | ||
| } | ||
|
|
||
| # Hash adapter configuration | ||
| adapter = scenario.adapter | ||
| hash_data["adapter"] = { | ||
| "name": getattr(adapter, "name", adapter.__class__.__name__), | ||
| "class": adapter.__class__.__name__, | ||
| } | ||
|
|
||
| # Hash dataset cases | ||
| try: | ||
| cases = list(scenario.dataset.iter_cases()) | ||
| hash_data["dataset"] = { | ||
| "name": scenario.dataset.name, | ||
| "cases": [ | ||
| { | ||
| "id": case.id, | ||
| "query": case.query, | ||
| "expected_output": case.expected_output, | ||
| } | ||
| for case in cases | ||
| ], | ||
| } | ||
| except Exception: | ||
| # If we can't iterate cases, use dataset name only | ||
| hash_data["dataset"] = {"name": scenario.dataset.name} | ||
|
|
||
| # Create deterministic JSON string and hash it | ||
| json_str = json.dumps(hash_data, sort_keys=True, default=str) | ||
| return hashlib.sha256(json_str.encode()).hexdigest()[:16] | ||
|
|
There was a problem hiding this comment.
Consider collision risk with truncated hash.
The cache key uses only the first 16 hex characters (64 bits) of the SHA256 hash. While this reduces file name length, it increases collision probability following the birthday paradox. With approximately 2^32 scenarios, collisions become likely, which could cause incorrect cache hits where one scenario uses another's cached result.
🔎 Suggested fix: Use full hash or longer prefix
# Create deterministic JSON string and hash it
json_str = json.dumps(hash_data, sort_keys=True, default=str)
- return hashlib.sha256(json_str.encode()).hexdigest()[:16]
+ return hashlib.sha256(json_str.encode()).hexdigest() # Use full 64-char hashAlternatively, if shorter names are desired, use at least 32 characters (128 bits) to maintain collision safety for billions of scenarios.
🤖 Prompt for AI Agents
In src/agentunit/pytest/cache.py around lines 49 to 90, the scenario cache key
truncates the SHA256 to 16 hex chars (64 bits) which raises a real collision
risk; change the returned hash to a longer value (recommended at least 32 hex
chars / 128 bits, or use the full SHA256 hex) to reduce collisions, and update
any consumers or filename/length constraints that assumed 16 chars to accept the
longer string.
| click.echo(f"\nEvaluation directory initialized at {eval_dir}") | ||
| click.echo("\nNext steps:") | ||
| click.echo(f"1. Add scenario files to {eval_dir}/") | ||
| click.echo("2. Run: pytest tests/eval/") | ||
| click.echo("3. See docs/pytest-plugin.md for more information") |
There was a problem hiding this comment.
"Next steps" output uses hardcoded path instead of the user-provided directory.
When a user specifies a custom --directory, the output still says pytest tests/eval/ instead of using the actual directory.
🔎 Proposed fix
click.echo(f"\nEvaluation directory initialized at {eval_dir}")
click.echo("\nNext steps:")
click.echo(f"1. Add scenario files to {eval_dir}/")
- click.echo("2. Run: pytest tests/eval/")
+ click.echo(f"2. Run: pytest {eval_dir}/")
click.echo("3. See docs/pytest-plugin.md for more information")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| click.echo(f"\nEvaluation directory initialized at {eval_dir}") | |
| click.echo("\nNext steps:") | |
| click.echo(f"1. Add scenario files to {eval_dir}/") | |
| click.echo("2. Run: pytest tests/eval/") | |
| click.echo("3. See docs/pytest-plugin.md for more information") | |
| click.echo(f"\nEvaluation directory initialized at {eval_dir}") | |
| click.echo("\nNext steps:") | |
| click.echo(f"1. Add scenario files to {eval_dir}/") | |
| click.echo(f"2. Run: pytest {eval_dir}/") | |
| click.echo("3. See docs/pytest-plugin.md for more information") |
🤖 Prompt for AI Agents
In src/agentunit/pytest/cli.py around lines 39 to 43, the "Next steps" message
prints a hardcoded pytest path ("pytest tests/eval/") instead of using the
user-provided eval_dir; update the message to interpolate the actual eval_dir
(e.g., "pytest {eval_dir}/") or construct the path with pathlib to ensure
correct formatting so the printed command reflects the chosen directory.
| def pytest_collect_file(file_path: Path, parent: Collector) -> Module | None: | ||
| """Collect AgentUnit scenario files as pytest tests.""" | ||
| # Only collect files in tests/eval/ directory | ||
| if not _is_eval_directory(file_path): | ||
| return None | ||
|
|
||
| # Look for scenario files (Python files or YAML/JSON configs) | ||
| if file_path.suffix in {".py", ".yaml", ".yml", ".json"}: | ||
| return AgentUnitFile.from_parent(parent, path=file_path) | ||
|
|
||
| return None | ||
|
|
||
|
|
||
| def _is_eval_directory(file_path: Path) -> bool: | ||
| """Check if file is in tests/eval/ directory.""" | ||
| parts = file_path.parts | ||
| return "tests" in parts and "eval" in parts | ||
|
|
There was a problem hiding this comment.
Path check is too permissive and may collect unintended files.
The _is_eval_directory function checks if both "tests" and "eval" appear anywhere in the path parts, which could match unintended directories like mytests/foo/eval, tests/integration/eval, or even eval/tests. This may lead to discovering scenarios from unexpected locations.
🔎 Suggested fix: Check for consecutive "tests/eval" pattern
def _is_eval_directory(file_path: Path) -> bool:
"""Check if file is in tests/eval/ directory."""
parts = file_path.parts
- return "tests" in parts and "eval" in parts
+ # Check for consecutive "tests" followed by "eval"
+ for i in range(len(parts) - 1):
+ if parts[i] == "tests" and parts[i + 1] == "eval":
+ return True
+ return FalseThis ensures the file is specifically under a tests/eval/ directory structure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def pytest_collect_file(file_path: Path, parent: Collector) -> Module | None: | |
| """Collect AgentUnit scenario files as pytest tests.""" | |
| # Only collect files in tests/eval/ directory | |
| if not _is_eval_directory(file_path): | |
| return None | |
| # Look for scenario files (Python files or YAML/JSON configs) | |
| if file_path.suffix in {".py", ".yaml", ".yml", ".json"}: | |
| return AgentUnitFile.from_parent(parent, path=file_path) | |
| return None | |
| def _is_eval_directory(file_path: Path) -> bool: | |
| """Check if file is in tests/eval/ directory.""" | |
| parts = file_path.parts | |
| return "tests" in parts and "eval" in parts | |
| def pytest_collect_file(file_path: Path, parent: Collector) -> Module | None: | |
| """Collect AgentUnit scenario files as pytest tests.""" | |
| # Only collect files in tests/eval/ directory | |
| if not _is_eval_directory(file_path): | |
| return None | |
| # Look for scenario files (Python files or YAML/JSON configs) | |
| if file_path.suffix in {".py", ".yaml", ".yml", ".json"}: | |
| return AgentUnitFile.from_parent(parent, path=file_path) | |
| return None | |
| def _is_eval_directory(file_path: Path) -> bool: | |
| """Check if file is in tests/eval/ directory.""" | |
| parts = file_path.parts | |
| # Check for consecutive "tests" followed by "eval" | |
| for i in range(len(parts) - 1): | |
| if parts[i] == "tests" and parts[i + 1] == "eval": | |
| return True | |
| return False |
🤖 Prompt for AI Agents
In src/agentunit/pytest/plugin.py around lines 69 to 86, the _is_eval_directory
check is too permissive because it merely tests that "tests" and "eval" appear
anywhere in the path parts; update it to only return True when the parts contain
the consecutive segment "tests" immediately followed by "eval" (i.e., scan parts
for an index i where parts[i] == "tests" and parts[i+1] == "eval"), so files are
only collected when they are actually under a tests/eval/... directory
structure; implement this consecutive-pair check and handle bounds safely.
| def _discover_config_scenarios(self) -> list[Scenario]: | ||
| """Discover scenarios from config files.""" | ||
| try: | ||
| from agentunit.nocode import ScenarioBuilder | ||
|
|
||
| builder = ScenarioBuilder.from_file(self.path) | ||
| scenario = builder.to_scenario() | ||
| return [scenario] | ||
| except ImportError: | ||
| return [] | ||
| except Exception: | ||
| return [] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Silent exception handling hides configuration errors.
Lines 158-159 catch all exceptions when loading config files and silently return an empty list. If a YAML/JSON file has syntax errors or invalid scenario configuration, users won't see any error message, making debugging difficult.
🔎 Suggested fix: Log configuration errors
def _discover_config_scenarios(self) -> list[Scenario]:
"""Discover scenarios from config files."""
try:
from agentunit.nocode import ScenarioBuilder
builder = ScenarioBuilder.from_file(self.path)
scenario = builder.to_scenario()
return [scenario]
except ImportError:
+ logger.debug("nocode module not available, skipping config file")
return []
- except Exception:
+ except Exception as e:
+ logger.error(f"Failed to load config from {self.path}: {e}")
return []Alternatively, raise the exception and let the collect() method handle it via the load_error mechanism, which would create a visible failing test.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/agentunit/pytest/plugin.py around lines 148 to 160, the broad except
Exception that returns an empty list silently hides config parsing errors;
replace it so errors are surfaced: either catch Exception as e and log the
exception (including message and traceback) before returning an empty list, or
preferably re-raise the exception (raise) so pytest's collect/load_error
machinery reports a visible failing test; implement one of these approaches and
remove the silent bare except block.
| def _import_module(self) -> Any: | ||
| """Import Python module from file path.""" | ||
| try: | ||
| import importlib.util | ||
| import sys | ||
|
|
||
| spec = importlib.util.spec_from_file_location(self.path.stem, self.path) | ||
| if spec is None or spec.loader is None: | ||
| return None | ||
|
|
||
| module = importlib.util.module_from_spec(spec) | ||
| sys.modules[self.path.stem] = module | ||
| spec.loader.exec_module(module) | ||
| return module | ||
| except Exception: | ||
| return None | ||
|
|
There was a problem hiding this comment.
Module name conflicts could occur with same-named files.
Line 172 adds the module to sys.modules using only the file stem (e.g., "example_scenarios"). If multiple test files have the same name in different directories (e.g., tests/eval/foo/scenarios.py and tests/eval/bar/scenarios.py), they will conflict in sys.modules, causing the later import to overwrite the earlier one.
🔎 Suggested fix: Use unique module names
def _import_module(self) -> Any:
"""Import Python module from file path."""
try:
import importlib.util
import sys
+ # Create unique module name to avoid conflicts
+ module_name = f"agentunit.eval.{self.path.stem}_{id(self.path)}"
- spec = importlib.util.spec_from_file_location(self.path.stem, self.path)
+ spec = importlib.util.spec_from_file_location(module_name, self.path)
if spec is None or spec.loader is None:
return None
module = importlib.util.module_from_spec(spec)
- sys.modules[self.path.stem] = module
+ sys.modules[module_name] = module
spec.loader.exec_module(module)
return module
except Exception:
return NoneUsing a unique name (including file path or object ID) prevents conflicts between same-named files in different directories.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _import_module(self) -> Any: | |
| """Import Python module from file path.""" | |
| try: | |
| import importlib.util | |
| import sys | |
| spec = importlib.util.spec_from_file_location(self.path.stem, self.path) | |
| if spec is None or spec.loader is None: | |
| return None | |
| module = importlib.util.module_from_spec(spec) | |
| sys.modules[self.path.stem] = module | |
| spec.loader.exec_module(module) | |
| return module | |
| except Exception: | |
| return None | |
| def _import_module(self) -> Any: | |
| """Import Python module from file path.""" | |
| try: | |
| import importlib.util | |
| import sys | |
| # Create unique module name to avoid conflicts | |
| module_name = f"agentunit.eval.{self.path.stem}_{id(self.path)}" | |
| spec = importlib.util.spec_from_file_location(module_name, self.path) | |
| if spec is None or spec.loader is None: | |
| return None | |
| module = importlib.util.module_from_spec(spec) | |
| sys.modules[module_name] = module | |
| spec.loader.exec_module(module) | |
| return module | |
| except Exception: | |
| return None |
🤖 Prompt for AI Agents
In src/agentunit/pytest/plugin.py around lines 161 to 177, adding the module to
sys.modules using only self.path.stem can cause name collisions for same-named
files; instead generate a unique module name (for example by including the
file's resolved path, a checksum/hash of the path, or a UUID) and use that
unique name when creating and registering the module in sys.modules and when
creating the spec/module, so each imported file gets a distinct module key and
avoids overwriting another import.
Signed-off-by: Siddhant Shekhar <shekharsiddhant93@gmail.com>
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/pytest-plugin.md (2)
42-58: Fix malformed and duplicate SimpleAdapter class definition.The example class has critical issues that would prevent it from working:
- Lines 50-58 duplicate the entire class definition immediately after the first definition (lines 42-48)
- Line 50 places a docstring after the class header (invalid Python syntax placement)
- The class methods are repeated
This example will confuse users and won't execute as written.
🔎 Proposed fix: Single, correct class definition
class SimpleAdapter(BaseAdapter): - name = "simple" - - def __init__(self, agent_func): - self.agent_func = agent_func - - def prepare(self): - pass - - """Simple adapter for function-based agents.""" - name = "simple" + """Simple adapter for function-based agents.""" def __init__(self, agent_func): self.agent_func = agent_func def prepare(self): pass def execute(self, case, trace):
263-266: Address orphaned code block.Lines 263-266 contain a code block mentioning "# Run with coverage" that appears disconnected from any surrounding context or section. Either integrate it into an appropriate section with proper explanation or remove it.
🧹 Nitpick comments (1)
docs/pytest-plugin.md (1)
231-234: Specify language for fenced code block.The gitignore example block is missing a language identifier. Add
gitignoreto the opening fence for proper syntax highlighting.-```gitignore +```gitignore # AgentUnit cache .agentunit_cache/</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f624e846814de0effb860a4e27abbee95dd7539b and be92367f9485012f67a1af038feb4b8a159b5e69. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/pytest-plugin.md` (4 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/pytest-plugin.md</summary> 251-251: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: Test (Python 3.10) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ## Configuration | ||
|
|
||
| Add pytest configuration in `pyproject.toml`: | ||
|
|
||
| ```toml | ||
| [tool.pytest.ini_options] | ||
| markers = [ | ||
| "agentunit: marks test as an AgentUnit scenario evaluation", | ||
| "scenario(name): marks test with specific scenario name", | ||
| ] | ||
| testpaths = ["tests", "tests/eval"] | ||
| ``` | ||
|
|
||
| ## Example Directory Structure | ||
|
|
||
| ``` | ||
| project/ | ||
| ├── tests/ | ||
| │ ├── eval/ # AgentUnit scenarios | ||
| │ │ ├── __init__.py | ||
| │ │ ├── basic_scenarios.py | ||
| │ │ └── advanced_scenarios.py | ||
| │ └── test_regular.py | ||
| ├── src/ | ||
| │ └── myproject/ | ||
| └── pyproject.toml | ||
| ``` |
There was a problem hiding this comment.
Remove duplicate Configuration and Example Directory Structure sections.
These sections (lines 236-247 and 249-262) are duplicates of earlier sections in the file (lines 148-175), creating redundancy and confusion for readers. Keep only the first occurrence and remove the duplicates.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
251-251: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/pytest-plugin.md around lines 236 to 262, the "Configuration" and
"Example Directory Structure" blocks are duplicates of the earlier occurrences
(lines ~148-175); remove the duplicate blocks (lines 236-247 and 249-262) so
only the first occurrence remains, ensure surrounding spacing and headings
remain consistent after deletion, and update any local references or anchors if
necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/agentunit/pytest/plugin.py (3)
79-82: Path check remains too permissive.This issue was flagged in a previous review. The current check still matches unintended directories like
eval/testsortests/integration/eval.
152-153: Silent exception handling still present.This issue was flagged in a previous review. Config file parsing errors are silently swallowed, making debugging difficult.
161-167: Module name collision risk still present.This issue was flagged in a previous review. Using
self.path.stemas the module name can cause conflicts with same-named files in different directories.
🧹 Nitpick comments (3)
tests/test_pytest_plugin.py (2)
335-363: Consider asserting exact count for deterministic behavior.Line 362 uses
assert count >= 1, but since only one cache entry is set in this test, you could assertcount == 1for a more precise assertion. This would catch potential issues where more entries are being cleared than expected.🔎 Suggested fix
- count = cache.clear() - assert count >= 1 + count = cache.clear() + assert count == 1
396-446: Consider adding test fixture to reduce boilerplate.All 6 tests repeat similar DatasetSource/Scenario setup. A pytest fixture could reduce duplication and improve maintainability.
@pytest.fixture def make_scenario(): """Factory fixture for creating test scenarios.""" def _make(name: str, expected_output: str = "hi"): class TestDataset(DatasetSource): def __init__(self): super().__init__( name=name, loader=lambda: [DatasetCase(id="test1", query="hello", expected_output=expected_output)], ) def test_agent(payload): return {"result": expected_output} return Scenario( name=f"{name}-scenario", adapter=SimpleTestAdapter(test_agent), dataset=TestDataset(), ) return _makesrc/agentunit/pytest/plugin.py (1)
186-186: Defensive check for parent is unnecessary.The
parentparameter is typed asAgentUnitFile(non-optional) and is always provided viafrom_parent(). Theif parent else Nonecheck is redundant but harmless.🔎 Suggested simplification
- self._source_path = parent.path if parent else None + self._source_path = parent.path
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/agentunit/pytest/cache.py(1 hunks)src/agentunit/pytest/plugin.py(6 hunks)tests/test_pytest_plugin.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/agentunit/pytest/cache.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/agentunit/pytest/plugin.py (3)
src/agentunit/core/scenario.py (1)
Scenario(23-278)src/agentunit/core/runner.py (1)
run_suite(101-111)src/agentunit/pytest/cache.py (3)
ScenarioCache(33-192)clear(178-192)get(107-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (Python 3.12)
🔇 Additional comments (8)
tests/test_pytest_plugin.py (3)
236-270: Tests cover the core cache lifecycle well.The test correctly validates cache creation, storage, retrieval, and data integrity. The assertions on
cached.successandcached.failuresverify the round-trip behavior.
272-297: Disabled cache behavior verified correctly.Testing that
cache.set()returns an empty string andcache.get()returnsNonewhen disabled ensures the no-op behavior is properly enforced.
299-333: Cache invalidation test is well-designed.The test correctly verifies that modifying the source file content invalidates the cache entry. This is critical for ensuring stale results aren't served.
src/agentunit/pytest/plugin.py (5)
28-44: CLI options are well-defined.The
--no-cacheand--clear-cacheoptions follow pytest conventions with proper group, action, and help text. Thedestparameter ensures consistent attribute naming.
52-63: Cache initialization and clear logic looks correct.The cache is properly initialized based on the
--no-cacheflag and stored on config. The clear operation correctly runs before tests if requested.One minor note: the
if count > 0check (line 62) is redundant since logging "Cleared 0 cached results" would also be acceptable, but the current approach avoids noise.
195-198: Cache accessor is clean and safe.Using
getattrwith a default ofNoneensures graceful handling if the cache wasn't initialized (e.g., during standalone testing).
207-218: Cache hit handling correctly differentiates success and failure.The logic properly returns early on cached success and re-raises the original failure message on cached failure. This ensures consistent test output between fresh runs and cached runs.
233-241: Cache storage handles both success and failure cases.Results are properly stored with the source path for future invalidation checks. The logging distinguishes between success and failure caching.
|
Hello @aviralgarg05 , I’ve opened a new PR in the correct repository with the same changes. |
…viralgarg05#46) * feat: add pytest plugin with result caching for unchanged scenarios (aviralgarg05#26) - Add pytest plugin for AgentUnit scenario discovery and execution - Add ScenarioCache class to hash scenario inputs and store results - Store cache in .agentunit_cache/ directory with auto .gitignore - Add --no-cache flag to bypass cache and force fresh runs - Add --clear-cache flag to clear cache before running tests - Invalidate cache when source files change - Add comprehensive tests for plugin and caching functionality - Add documentation for pytest plugin usage * fix: resolve ruff linting errors in pytest plugin * fix: add missing cache tests and format code --------- Signed-off-by: Siddhant Shekhar <shekharsiddhant93@gmail.com>
3 participants
Summary
#26
Implements the pytest plugin with result caching to skip re-running unchanged scenarios, as requested in #26.
Changes
New:
src/agentunit/pytest/- Complete pytest plugin moduleplugin.py- Scenario discovery and executioncache.py- Result caching with hash-based keyscli.py- CLI for initializing eval directoriesNew:
tests/eval/- Example scenarios directoryNew:
tests/test_pytest_plugin.py- 12 tests for plugin + cachingNew:
docs/pytest-plugin.md- DocumentationFeatures
--no-cacheflag to bypass cache and force fresh runs--clear-cacheflag to clear cache before running.agentunit_cache/with auto.gitignoreUsage