fix: Remove pytest_plugins from conftest to resolve pytest error - #3452
fix: Remove pytest_plugins from conftest to resolve pytest error#3452Hore01 wants to merge 2 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
Greptile SummaryThe PR updates the nested pytest configuration.
Confidence Score: 3/5The PR is not yet safe to merge because the revised core SDK path still breaks imports in an uninstalled source checkout. The safety plugins are now explicitly loaded during pytest configuration, resolving the earlier registration issue, but the core SDK path traverses past Files Needing Attention: src/praisonai/tests/conftest.py
|
| Filename | Overview |
|---|---|
| src/praisonai/tests/conftest.py | Replaces nested pytest_plugins declarations with explicit plugin imports and marker registration, while revising local package path setup. |
Reviews (2): Last reviewed commit: "fix: resolve pytest_plugins error with p..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe test configuration registers custom pytest markers and conditionally adds valid, non-duplicate directories to ChangesPytest configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/blocked:cooldown
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3452Verdict: ✅ Approved after fixes (pushed to this branch)
Architecture review (AGENTS.md)
Issues confirmed & fixedBoth Greptile P1 findings were valid. The original PR fixed the pytest error but introduced two regressions: 1. Test safety plugins were silently disabled. Removing 2. Core SDK path was wrong. Verification (pytest 9.1.1 in a clean env)
Files modified
Skipped
|
|
@claude CI failed on HEAD Failed checks
Failures (extracted)
Critical review firstBefore changing code or tests, decide which side is wrong:
SDK guardrails (AGENTS.md):
What to do
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai/tests/conftest.py`:
- Around line 9-27: Update pytest_configure and the repository-root conftest
setup to ensure tests._pytest_plugins.test_gating and
tests._pytest_plugins.network_guard remain loaded as pytest plugins, either by
preserving the root conftest plugin registration or explicitly registering both
modules through pytest’s plugin manager. Keep the existing custom marker
registrations unchanged.
- Around line 59-63: Correct the `_agents_path` construction in the test path
setup to use two parent-directory segments, resolving to the repository’s
`src/praisonai-agents` SDK root. Keep the existing existence check and
`sys.path.insert` behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bbafbda4-0c7b-4277-a649-9442822a4675
📒 Files selected for processing (1)
src/praisonai/tests/conftest.py
| # Correct paths for src/praisonai/tests/conftest.py | ||
| # Go up 3 levels to reach root (tests -> praisonai -> src -> root) | ||
| _agents_path = os.path.join(os.path.dirname(__file__), '..', '..', '..', 'praisonai-agents') | ||
| if os.path.exists(_agents_path) and _agents_path not in sys.path: | ||
| sys.path.insert(0, _agents_path) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files of interest =="
git ls-files | rg '(^|/)conftest\.py$|praisonai-agents/|src/praisonai/agents|ARCHITECTURE\.md' || true
echo
echo "== conftest relevant lines =="
if [ -f src/praisonai/tests/conftest.py ]; then
wc -l src/praisonai/tests/conftest.py
sed -n '1,90p' src/praisonai/tests/conftest.py | cat -n
fi
echo
echo "== path existence and resolution probe =="
python3 - <<'PY'
import os
print("cwd:", os.getcwd())
print("repo root files containing praisonai-agents:")
for root, dirs, files in os.walk("."):
dirs[:] = [d for d in dirs if not d.startswith('.')]
if 'praisonai-agents' in dirs:
full = os.path.join(root, 'praisonai-agents')
print("dir", full, "exists", os.path.exists(full), "isdir", os.path.isdir(full))
if 'praisonai' in dirs:
full = os.path.join(root, 'praisonai')
print("dir", full, "exists", os.path.exists(full), "isdir", os.path.isdir(full))
conftest = "/app/src/praisonai/tests/conftest.py"
if os.path.exists(conftest):
conftest = conftest
elif os.path.exists("src/praisonai/tests/conftest.py"):
conftest = "src/praisonai/tests/conftest.py"
else:
conftest = None
print("conftest_path", conftest)
if conftest:
dirname = os.path.dirname(conftest)
for k in range(1, 5):
p = os.path.join(dirname, *([".."] * k), "praisonai-agents")
print("k", k, "rel", p, "abs", os.path.abspath(p), "exists", os.path.exists(p), "isdir", os.path.isdir(p))
p = os.path.join(dirname, "..", "..", "..", "praisonai-agents")
normalized = os.path.normpath(p)
print("three normalized", normalized)
print("src relative normalized", os.path.normpath(os.path.join(dirname, "..", "..", "praisonai-agents")))
PYRepository: MervinPraison/PraisonAI
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/praisonai/tests/conftest.py =="
wc -l src/praisonai/tests/conftest.py
sed -n '1,90p' src/praisonai/tests/conftest.py | cat -n
echo
echo "== src/praisonai-agents/praisonaiagents imports =="
rg -n "from praisonaiagents|import praisonaiagents|__init__" src/praisonai-agents/praisonaiagents src/praisonai-agents/__init__.py | head -120
echo
echo "== path probe from file path =="
python3 - <<'PY'
import os
conftest = "src/praisonai/tests/conftest.py"
dirname = os.path.dirname(conftest)
for k in range(1, 5):
p = os.path.join(dirname, *([".."] * k), "praisonai-agents")
print(f"{'..'*k}/praisonai-agents -> {os.path.abspath(p)} -> exists={os.path.exists(p)} isdir={os.path.isdir(p)}")
print("three normalized:", os.path.normpath(os.path.join(dirname, "..", "..", "..", "praisonai-agents")))
print("two normalized:", os.path.normpath(os.path.join(dirname, "..", "..", "praisonai-agents")))
PY
echo
echo "== wrapper package structure relevant =="
sed -n '1,120p' src/praisonai/__init__.py 2>/dev/null | cat -n || true
rg -n "^from praisonaiagents|^import praisonaiagents" src/praisonai -g '*.py' | head -80
rg -n "sys\.path\.insert|praisonai-agents" src/praisonai -g '*.py' | head -80Repository: MervinPraison/PraisonAI
Length of output: 36691
Correct _agents_path to the SDK root.
From src/praisonai/tests/conftest.py, two .. segments reach <repo>/src/praisonai-agents, while _agents_path goes one level too high. That makes the os.path.exists(...) guard skip an unavailable root-level path, so uninstalled source-checkout tests can fail when importing praisonaiagents.
Proposed fix
-_agents_path = os.path.join(os.path.dirname(__file__), '..', '..', '..', 'praisonai-agents')
+_agents_path = os.path.abspath(
+ os.path.join(os.path.dirname(__file__), '..', '..', 'praisonai-agents')
+)**
📝 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.
| # Correct paths for src/praisonai/tests/conftest.py | |
| # Go up 3 levels to reach root (tests -> praisonai -> src -> root) | |
| _agents_path = os.path.join(os.path.dirname(__file__), '..', '..', '..', 'praisonai-agents') | |
| if os.path.exists(_agents_path) and _agents_path not in sys.path: | |
| sys.path.insert(0, _agents_path) | |
| # Correct paths for src/praisonai/tests/conftest.py | |
| # Go up 3 levels to reach root (tests -> praisonai -> src -> root) | |
| _agents_path = os.path.abspath( | |
| os.path.join(os.path.dirname(__file__), '..', '..', 'praisonai-agents') | |
| ) | |
| if os.path.exists(_agents_path) and _agents_path not in sys.path: | |
| sys.path.insert(0, _agents_path) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/tests/conftest.py` around lines 59 - 63, Correct the
`_agents_path` construction in the test path setup to use two parent-directory
segments, resolving to the repository’s `src/praisonai-agents` SDK root. Keep
the existing existence check and `sys.path.insert` behavior unchanged.
|
Claude finished @MervinPraison's task —— View job CI Failure Fix — PR #3452Verdict: Regression introduced by this PR — already fixed on branch HEAD (
Root causeThe 7 failing tests live in the pre-existing, stale These stale tests never ran in CI because the The original commit Fix (already on branch HEAD
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3452 +/- ##
==========================================
+ Coverage 32.40% 32.61% +0.20%
==========================================
Files 542 542
Lines 57172 57403 +231
==========================================
+ Hits 18528 18721 +193
- Misses 38644 38682 +38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
MervinPraison
removed
the
pipeline/blocked:cooldown
2 participants
Description
Fixes the pytest collection error caused by
pytest_pluginsbeing declared in a non-top-levelconftest.py, which is no longer supported in pytest 9.1.1+.Changes
pytest_pluginsdeclaration fromsrc/praisonai/tests/conftest.pypytest_configure()to register custom markers and eliminatePytestUnknownMarkWarningsys.pathimports for proper package resolutionconftest.pyin its original location to preserve the existing project structureTesting
Created a validation test (
src/praisonai/tests/test_conftest_validation.py) covering all conftest fixtures.Test Results
$ pytest src/praisonai/tests/test_conftest_validation.py -v -s ✅ test_conftest_loaded PASSED ✅ test_event_loop_works PASSED ✅ test_mock_llm_response_works PASSED ✅ test_sample_agent_config_works PASSED ✅ test_setup_test_environment_works PASSED ✅ test_fast_sleep_works PASSED ==================================== 6 passed in 0.28s ====================================Verification
pytest_pluginscollection errorsPytestUnknownMarkWarningTest Commands
Impact
This PR resolves the pytest collection failure introduced by pytest 9.1.1+, preserves the existing test structure, and ensures all test fixtures continue to work without breaking existing functionality.
Summary by CodeRabbit