fix: Fix n8n integration test mocking issues#21
Conversation
- Fix test_n8n_workflow_missing_httpx to use sys.modules approach for lazy import mocking - Enhance mock_httpx fixture with proper HTTPStatusError and TimeoutException classes - All 14 tests now pass (12 passed, 2 integration tests correctly skipped) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Fixes failing n8n integration unit tests by improving how the lazy httpx import is mocked and by providing mock exception types that match what the implementation catches.
Changes:
- Updated the
mock_httpxfixture to provideTimeoutExceptionandHTTPStatusErrortypes compatible with the n8n tool’s exception handling. - Switched the “missing httpx” test to a
sys.modules-based approach for simulating an unavailable dependency.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| mock_module.HTTPStatusError = MockHTTPStatusError | ||
|
|
||
| with patch.dict('sys.modules', {'httpx': mock_module}) as mock_modules: |
There was a problem hiding this comment.
patch.dict('sys.modules', {'httpx': mock_module}) as mock_modules binds mock_modules but it’s never used, which adds noise (and can trip linters). Drop the as mock_modules binding (or use it) to keep the fixture minimal.
| with patch.dict('sys.modules', {'httpx': mock_module}) as mock_modules: | |
| with patch.dict('sys.modules', {'httpx': mock_module}): |
| with patch.dict('sys.modules', {'httpx': None}): | ||
| with patch('builtins.__import__', side_effect=ImportError("No module named 'httpx'")): | ||
| result = tool.run(workflow_id="test-workflow") |
There was a problem hiding this comment.
Patching builtins.__import__ with a blanket ImportError makes this test overly broad: if tool.run() (or anything it calls) adds any other import, the test will still pass even though the failure wasn’t specifically due to missing httpx. Prefer relying on the sys.modules['httpx']=None behavior alone, or make the __import__ side effect conditional so it only fails when importing httpx.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the n8n integration tests by improving the httpx mock fixture with specific exception classes and updating the simulation of missing dependencies using sys.modules. A review comment suggests that patching builtins.__import__ is redundant when sys.modules['httpx'] is already set to None, recommending its removal to simplify the test and avoid potential side effects.
| tool = N8nWorkflowTool() | ||
| with patch('praisonai_tools.n8n.n8n_workflow.httpx', None): | ||
| # Mock the import failure using sys.modules approach | ||
| with patch.dict('sys.modules', {'httpx': None}): |
There was a problem hiding this comment.
The patch on builtins.__import__ (line 86) is redundant when sys.modules['httpx'] is set to None. Setting a module to None in sys.modules is the standard way to simulate a missing module in Python and will cause an ImportError immediately upon an import httpx statement. Removing the redundant patch simplifies the test and avoids the potential side effects of patching a core builtin like __import__.
Fixes #20
This PR fixes the test mocking issues in the n8n integration that were causing 2 test failures.
Changes
test_n8n_workflow_missing_httpxto usesys.modulesapproach for lazy import mockingmock_httpxfixture with proper HTTPStatusError and TimeoutException classesGenerated with Claude Code
Summary by CodeRabbit