Skip to content

Conversation

@filiplajszczak
Copy link

@filiplajszczak filiplajszczak commented Oct 22, 2025

These tests only exercise BedrockLLM._normalize_message, but the shared llm fixture bootstrapped a real Bedrock client and demanded AWS credentials. That made the “not integration” target fail despite filtering integration markers. Dropping the unused fixture keeps the tests offline.

Summary by CodeRabbit

  • Tests
    • Simplified test method signatures in AWS plugin tests through improved pytest fixture injection.

Note: This is an internal test refactoring with no user-facing changes.

These tests only exercise BedrockLLM._normalize_message, but the shared llm
fixture bootstrapped a real Bedrock client and demanded AWS credentials. That
made the “not integration” target fail despite filtering integration markers.
Dropping the unused fixture keeps the tests offline.
@coderabbitai
Copy link

coderabbitai bot commented Oct 22, 2025

Walkthrough

Two test methods in TestBedrockLLM class have their pytest signatures simplified by removing explicit llm fixture parameters. The methods now rely on pytest fixture injection instead, with no changes to test logic or behavior.

Changes

Cohort / File(s) Summary
Test method signature simplification
plugins/aws/tests/test_aws.py
Removed explicit llm: BedrockLLM parameter from test_message() and test_advanced_message() methods in TestBedrockLLM class, leveraging pytest fixture injection instead

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

The fixtures dissolve into air,
no longer named, no longer held,
yet still they arrive—
invisible thread, invisible grace.
What was explicit becomes faith.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "test(aws): run normalize tests without Bedrock fixture" directly and clearly describes the main change in the changeset. The title accurately reflects that the llm fixture parameter is being removed from the two test methods, allowing them to run without requiring the Bedrock fixture and AWS credentials. The title is concise, specific, and uses clear language without vague terms or noise. A teammate scanning the commit history would immediately understand that this change removes a fixture dependency from AWS tests.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9c9737f and 92bdf9b.

📒 Files selected for processing (1)
  • plugins/aws/tests/test_aws.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (.cursor/rules/python.mdc)

**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide

Files:

  • plugins/aws/tests/test_aws.py
🧬 Code graph analysis (1)
plugins/aws/tests/test_aws.py (1)
plugins/aws/vision_agents/plugins/aws/aws_llm.py (1)
  • _normalize_message (449-480)
🔇 Additional comments (3)
plugins/aws/tests/test_aws.py (3)

52-57: LGTM! Correctly removes unused fixture dependency.

The test only calls BedrockLLM._normalize_message() as a static method and never uses an llm instance. Removing the fixture parameter allows this test to run offline without AWS credentials, perfectly aligning with the PR objective.


60-66: LGTM! Correctly removes unused fixture dependency.

The test only calls BedrockLLM._normalize_message() as a static method and never uses an llm instance. Removing the fixture parameter allows this test to run offline without AWS credentials, perfectly aligning with the PR objective.


52-66: AI summary contains a factual inaccuracy.

The AI-generated summary states these methods "now rely on pytest fixture injection" after the change. This is incorrect—the changes remove the fixture dependency entirely because both tests only invoke the static method BedrockLLM._normalize_message(), which requires neither an instance nor AWS credentials.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant