Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
.env
.vscode
.DS_Store
*_creds*
*_creds*
**/venv/
**/__pycache__/
Comment on lines +4 to +6
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider adding a root-level .venv/ pattern and byte-code glob

**/venv/ covers nested folders, but the most common virtual-env name is .venv/ in the repo root.
Likewise, adding *.py[cod] ensures all compiled byte-code artefacts (including edge cases outside __pycache__) are ignored.

 **/venv/
+/.venv/
+*.py[cod]
 **/__pycache__/
📝 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.

Suggested change
*_creds*
**/venv/
**/__pycache__/
*_creds*
**/venv/
/.venv/
*.py[cod]
**/__pycache__/
🤖 Prompt for AI Agents
In the .gitignore file around lines 4 to 6, add a root-level `.venv/` pattern to
ignore the common virtual environment folder at the repo root, and add a
`*.py[cod]` pattern to ignore all Python compiled byte-code files, including
those outside `__pycache__`. This complements the existing `**/venv/` and
`**/__pycache__/` patterns for more comprehensive ignoring of virtual
environments and byte-code artifacts.

2 changes: 1 addition & 1 deletion docs/fallbacks.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ result, err := bifrost.ChatCompletionRequest(
2. **Model Selection**

- Use models with similar capabilities
- Consider model-specific features (e.g., function calling, streaming)
- Consider model-specific features (e.g., function calling)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Drop streaming from model-specific features
This update correctly removes the deprecated “streaming” fallback reference. Consider grepping for other “streaming” mentions in docs to avoid stale guidance.

🤖 Prompt for AI Agents
In docs/fallbacks.md at line 96, the deprecated "streaming" fallback reference
has been removed, but there may be other mentions of "streaming" in the
documentation. Search the entire docs directory for any remaining references to
"streaming" and remove or update them to prevent stale or misleading guidance.

- Account for different token limits and pricing

3. **Error Handling**
Expand Down
7 changes: 0 additions & 7 deletions tests/core-providers/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,6 @@ You can run specific scenarios across all providers:
# Test only chat completion
go test -v ./tests/core-providers/ -run "Chat"

# Test only streaming
go test -v ./tests/core-providers/ -run "Streaming"

# Test only function calling
go test -v ./tests/core-providers/ -run "Function"
```
Expand Down Expand Up @@ -184,8 +181,6 @@ Each provider is tested against these scenarios when supported:
- Simple Chat Completion
- Multi-turn Chat Conversation
- Chat with System Message
- Streaming Chat Completion
- Streaming Text Completion
- Text Completion with Parameters
- Chat Completion with Parameters
- Error Handling (Invalid Model)
Expand All @@ -197,7 +192,6 @@ Each provider is tested against these scenarios when supported:
- **Automatic Function Calling**: OpenAI, Anthropic, Bedrock, Azure, Vertex, Mistral, Ollama
- **Vision/Image Analysis**: OpenAI, Anthropic, Bedrock, Azure, Vertex, Mistral (limited support for Cohere and Ollama)
- **Text Completion**: Legacy models only (most providers now focus on chat completion)
- **Advanced streaming features**: Provider-dependent implementation

## 📊 Understanding Test Output

Expand Down Expand Up @@ -266,7 +260,6 @@ func getProviderCapabilities(providerName string) ProviderCapabilities {
return ProviderCapabilities{
SupportsTextCompletion: true,
SupportsChatCompletion: true,
SupportsStreaming: true,
SupportsFunctionCalling: false, // Update based on provider
SupportsAutomaticFunctions: false,
SupportsVision: false,
Expand Down
120 changes: 120 additions & 0 deletions tests/transports-integrations/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
# Bifrost Python E2E Test Makefile
# Provides convenient commands for running tests

# Get the directory where this Makefile is located
SCRIPT_DIR := $(dir $(abspath $(lastword $(MAKEFILE_LIST))))

.PHONY: help install test test-all test-parallel test-verbose clean lint format check-env
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Expand .PHONY to include all targets & set a default goal.

The current .PHONY omits newer targets (test-list, test-openai, test-anthropic, test-litellm, test-langchain, test-langgraph, test-mistral, test-genai, test-all, test-pytest-parallel, test-coverage, quick-test, all-tests, dev-setup). Also add:

.DEFAULT_GOAL := help

to guide users.

🧰 Tools
🪛 checkmake (0.2.2)

[warning] 4-4: Missing required phony target "all"

(minphony)

🤖 Prompt for AI Agents
In tests/transports-integrations/Makefile at line 4, the .PHONY declaration is
incomplete and missing several newer targets. Update the .PHONY line to include
all current targets such as test-list, test-openai, test-anthropic,
test-litellm, test-langchain, test-langgraph, test-mistral, test-genai,
test-all, test-pytest-parallel, test-coverage, quick-test, all-tests, and
dev-setup. Additionally, add the line .DEFAULT_GOAL := help at the top of the
Makefile to set the default goal and improve user guidance.


Comment on lines +7 to +8
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

.PHONY list is incomplete – several targets may collide with files

Add all public targets and a default goal:

.PHONY: help install check-env test test-parallel test-verbose test-list \
        test-openai test-anthropic test-litellm test-langchain test-langgraph \
        test-mistral test-genai test-all test-pytest-parallel test-coverage \
        lint format clean quick-test all-tests dev-setup
.DEFAULT_GOAL := help
🧰 Tools
🪛 checkmake (0.2.2)

[warning] 7-7: Missing required phony target "all"

(minphony)

🤖 Prompt for AI Agents
In tests/transports-integrations/Makefile around lines 7 to 8, the .PHONY
declaration is incomplete and may cause target-file collisions. Update the
.PHONY line to include all public targets listed in the comment, separated by
spaces and backslashes for line continuation. Also, add a .DEFAULT_GOAL
directive set to 'help' to specify the default make target.

# Default target
help:
@echo "Bifrost Python E2E Test Commands:"
@echo ""
@echo "Setup:"
@echo " install Install Python dependencies"
@echo " check-env Check environment variables"
@echo ""
@echo "Testing:"
@echo " test Run all tests using master runner"
@echo " test-all Run all tests with pytest"
@echo " test-parallel Run tests in parallel"
@echo " test-verbose Run tests with verbose output"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
@echo " test-openai Run OpenAI integration tests only"
@echo " test-anthropic Run Anthropic integration tests only"
@echo " test-litellm Run LiteLLM integration tests only"
@echo " test-langchain Run LangChain integration tests only"
@echo " test-langgraph Run LangGraph integration tests only"
@echo " test-mistral Run Mistral integration tests only"
@echo " test-genai Run Google GenAI integration tests only"
@echo ""
@echo "Development:"
@echo " lint Run code linting"
@echo " format Format code with black"
@echo " clean Clean up temporary files"

# Setup commands
install:
pip install -r $(SCRIPT_DIR)requirements.txt

check-env:
@echo "Checking environment variables..."
@python -c "import os; print('✓ BIFROST_BASE_URL:', os.getenv('BIFROST_BASE_URL', 'http://localhost:8080'))"
@python -c "import os; print('✓ OPENAI_API_KEY:', 'Set' if os.getenv('OPENAI_API_KEY') else 'Not set')"
@python -c "import os; print('✓ ANTHROPIC_API_KEY:', 'Set' if os.getenv('ANTHROPIC_API_KEY') else 'Not set')"
@python -c "import os; print('✓ MISTRAL_API_KEY:', 'Set' if os.getenv('MISTRAL_API_KEY') else 'Not set')"
@python -c "import os; print('✓ GOOGLE_API_KEY:', 'Set' if os.getenv('GOOGLE_API_KEY') else 'Not set')"

Comment on lines +39 to +46
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add checks for all provider API keys.

We currently verify OPENAI_API_KEY, ANTHROPIC_API_KEY, MISTRAL_API_KEY, and GOOGLE_API_KEY but miss others (e.g., LITELLM_API_KEY). Consider adding:

@python -c "import os; print('✓ LITELLM_API_KEY:', 'Set' if os.getenv('LITELLM_API_KEY') else 'Not set')"
🤖 Prompt for AI Agents
In tests/transports-integrations/Makefile around lines 36 to 43, the environment
variable checks miss some provider API keys like LITELLM_API_KEY. Add a similar
python command line to check for LITELLM_API_KEY by printing 'Set' if the
variable is present or 'Not set' if missing, following the existing pattern for
other API keys.

# Testing commands using master runner
test:
python $(SCRIPT_DIR)run_all_tests.py

test-parallel:
python $(SCRIPT_DIR)run_all_tests.py --parallel

test-verbose:
python $(SCRIPT_DIR)run_all_tests.py --verbose

test-list:
python $(SCRIPT_DIR)run_all_tests.py --list

# Individual integration tests
test-openai:
python $(SCRIPT_DIR)run_all_tests.py --integration openai --verbose

test-anthropic:
python $(SCRIPT_DIR)run_all_tests.py --integration anthropic --verbose

test-litellm:
python $(SCRIPT_DIR)run_all_tests.py --integration litellm --verbose

test-langchain:
python $(SCRIPT_DIR)run_all_tests.py --integration langchain --verbose

test-langgraph:
python $(SCRIPT_DIR)run_all_tests.py --integration langgraph --verbose

test-mistral:
python $(SCRIPT_DIR)run_all_tests.py --integration mistral --verbose

test-genai:
python $(SCRIPT_DIR)run_all_tests.py --integration genai --verbose

# Pytest commands
test-all:
pytest -v

test-pytest-parallel:
pytest -v -n auto

test-coverage:
pytest --cov=. --cov-report=html --cov-report=term

Comment on lines +83 to +91
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Scope pytest to the integration test folder.

Ensure pytest runs against the new tests directory:

-test-all:
-	pytest -v
+test-all:
+	pytest -v $(MAKEFILE_DIR)

This makes it explicit and avoids accidentally running unrelated tests.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In tests/transports-integrations/Makefile around lines 80 to 88, the pytest
commands run tests globally without scoping to the integration test folder.
Modify each pytest command to include the path to the integration test directory
explicitly, ensuring only tests in that folder are executed. This involves
appending the relative path of the integration tests directory to each pytest
command in the Makefile targets.

# Development commands
lint:
@echo "Running flake8..."
cd $(SCRIPT_DIR) && flake8 *.py
@echo "Running mypy..."
cd $(SCRIPT_DIR) && mypy *.py

format:
@echo "Formatting code with black..."
cd $(SCRIPT_DIR) && black *.py
Comment on lines +93 to +101
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Lint/format only top-level *.py; sub-packages are skipped

Replace the glob with a recursive path to cover the whole test suite:

lint:
	flake8 $(SCRIPT_DIR)

format:
	black $(SCRIPT_DIR)
🤖 Prompt for AI Agents
In tests/transports-integrations/Makefile around lines 93 to 101, the lint and
format targets only run on top-level Python files using the glob *.py, which
skips sub-packages. Update the commands to run flake8 and black recursively on
the entire $(SCRIPT_DIR) directory by replacing the glob with just $(SCRIPT_DIR)
as the argument. This ensures the whole test suite is covered.


clean:
@echo "Cleaning up temporary files..."
cd $(SCRIPT_DIR) && rm -rf __pycache__/
cd $(SCRIPT_DIR) && rm -rf .pytest_cache/
cd $(SCRIPT_DIR) && rm -rf .coverage
cd $(SCRIPT_DIR) && rm -rf htmlcov/
cd $(SCRIPT_DIR) && rm -rf .mypy_cache/
cd $(SCRIPT_DIR) && find . -name "*.pyc" -delete
cd $(SCRIPT_DIR) && find . -name "*.pyo" -delete

# Quick commands for common workflows
quick-test: check-env test

all-tests: install check-env test-parallel

dev-setup: install check-env
@echo "Development environment ready!"
@echo "Run 'make test' to execute all tests"
Comment on lines +113 to +120
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add workflow shortcuts to .PHONY.

Include quick-test, all-tests, and dev-setup in the phony list and ensure they reference $(MAKEFILE_DIR) where appropriate.

🤖 Prompt for AI Agents
In tests/transports-integrations/Makefile around lines 110 to 117, the new
workflow targets quick-test, all-tests, and dev-setup are missing from the
.PHONY declaration. Add these targets to the .PHONY list to prevent conflicts
with files of the same name. Also, update the commands in these targets to
reference $(MAKEFILE_DIR) where necessary to ensure they run in the correct
directory context.

Loading