Skip to content

chore: db fixture #432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 28, 2024
Merged

chore: db fixture #432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion tests/base_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from abc import ABC
from typing import Union

from selenium.webdriver import Chrome, Edge, Firefox
Expand All @@ -14,7 +15,7 @@
from utilities.vrt_helper import VrtHelper


class BaseTest:
class BaseTest(ABC):
driver: Union[Chrome, Firefox, Edge]
wait: WebDriverWait
about_page: AboutPage
Expand Down
9 changes: 9 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from _pytest.nodes import Item
from dotenv import load_dotenv
from mailinator import Mailinator
from mysql.connector import MySQLConnection
from requests_toolbelt.utils import dump
from selenium import webdriver
from selenium.webdriver.support.event_firing_webdriver import EventFiringWebDriver
Expand Down Expand Up @@ -100,6 +101,14 @@ def mailinator_helper() -> MailinatorHelper:
)


@pytest.fixture(scope="session")
def db_connection():
"""Fixture to establish a database connection."""
connection = MySQLConnection(user="root", password="1234", database="world")
yield connection
connection.close()

Comment on lines +104 to +110
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Security and configuration concerns in database fixture need to be addressed

The review comment is valid. The search results confirm:

  1. No environment configuration files exist in the project yet (.env, .yaml, etc.)
  2. The hardcoded credentials in tests/conftest.py are the only database configuration present
  3. The project uses mysql-connector-python package, confirming MySQL as the database

Suggested improvements remain valid:

  • Move credentials to environment variables
  • Add proper error handling
  • Enhance documentation
  • Add a sample .env.example file for test configuration

The original suggestion with environment variables and error handling should be implemented as proposed in the review comment.

🔗 Analysis chain

Security and configuration concerns in database fixture

Several issues need to be addressed:

  1. Hardcoded credentials pose a security risk and limit configuration flexibility
  2. Missing error handling for connection failures
  3. Documentation could be more comprehensive

Here's a suggested improvement:

 @pytest.fixture(scope="session")
 def db_connection():
-    """Fixture to establish a database connection."""
+    """Fixture to establish a MySQL database connection.
+    
+    Returns:
+        MySQLConnection: A connection to the MySQL database.
+        
+    Raises:
+        MySQLConnectionError: If connection cannot be established.
+    """
+    try:
+        connection = MySQLConnection(
+            host=os.getenv("DB_HOST", "localhost"),
+            user=os.getenv("DB_USER", "root"),
+            password=os.getenv("DB_PASSWORD"),
+            database=os.getenv("DB_NAME", "world")
+        )
+        yield connection
+    except Exception as e:
+        pytest.fail(f"Failed to establish database connection: {str(e)}")
+    finally:
+        if 'connection' in locals():
+            connection.close()
-    connection = MySQLConnection(user="root", password="1234", database="world")
-    yield connection
-    connection.close()

Let's check if environment variables are already defined in the project:

Let's search more broadly to understand the database configuration patterns in the project and any existing environment variable usage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for existing environment variable definitions
rg -l '(?:DB_HOST|DB_USER|DB_PASSWORD|DB_NAME)' --type-add 'env:*.env' -t env

Length of output: 228

Script:

#!/bin/bash
# Search for environment variable patterns in all files
rg -l '(?:DB_HOST|DB_USER|DB_PASSWORD|DB_NAME)'

# Look for any config or env sample files
fd -e env -e env.example -e env.sample -e cfg -e config -e ini -e yaml -e yml

# Check for existing database connection configurations
rg -i 'mysql.*connect|database.*connect'

Length of output: 5202


@pytest.fixture(scope="session")
def vrt_helper():
"""Fixture for creating a Visual Regression Tracker (VRT) helper object.
Expand Down
18 changes: 7 additions & 11 deletions tests/db_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,15 @@
import allure
import pytest
from assertpy import assert_that
from mysql.connector import MySQLConnection


@pytest.mark.skip(reason="requires database connection")
class TestDatabaseExample:
@allure.title("Verify population amounts")
def test_verify_population_amount(self):
with MySQLConnection(
user="root", password="1234", database="world"
) as connection:
cursor = connection.cursor()
cursor.execute("select Population from city where CountryCode='DNK'")
population_amount: list[int] = [item[0] for item in cursor.fetchall()]
assert_that(population_amount).described_as("population amount").is_equal_to(
[495699, 284846, 183912, 161161, 90327]
)
def test_verify_population_amount(self, db_connection):
with db_connection.cursor() as cursor:
cursor.execute("SELECT Population FROM city WHERE CountryCode='DNK'")
population_amount = [item[0] for item in cursor.fetchall()]
Comment on lines +10 to +12
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance database query security and error handling.

While the context manager usage is good, the SQL query should be parameterized to prevent SQL injection risks, even in tests. Additionally, consider adding error handling for database operations.

Consider this improvement:

 with db_connection.cursor() as cursor:
-    cursor.execute("SELECT Population FROM city WHERE CountryCode='DNK'")
+    try:
+        cursor.execute(
+            "SELECT Population FROM city WHERE CountryCode = %s",
+            ('DNK',)
+        )
+    except Exception as e:
+        pytest.fail(f"Database query failed: {str(e)}")
📝 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
with db_connection.cursor() as cursor:
cursor.execute("SELECT Population FROM city WHERE CountryCode='DNK'")
population_amount = [item[0] for item in cursor.fetchall()]
with db_connection.cursor() as cursor:
try:
cursor.execute(
"SELECT Population FROM city WHERE CountryCode = %s",
('DNK',)
)
population_amount = [item[0] for item in cursor.fetchall()]
except Exception as e:
pytest.fail(f"Database query failed: {str(e)}")

assert_that(population_amount).described_as(
"population amount"
).is_equal_to([495699, 284846, 183912, 161161, 90327])
Comment on lines +13 to +15
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider making population assertions more maintainable.

The hardcoded population values might become outdated and the test doesn't verify the order of results. Consider making the test more robust and maintainable.

Consider these improvements:

-            assert_that(population_amount).described_as(
-                "population amount"
-            ).is_equal_to([495699, 284846, 183912, 161161, 90327])
+            # Verify we have the expected number of cities
+            assert_that(population_amount).described_as(
+                "number of Danish cities"
+            ).is_length(5)
+            
+            # Verify population values are in descending order
+            sorted_population = sorted(population_amount, reverse=True)
+            assert_that(population_amount).described_as(
+                "population amounts in descending order"
+            ).is_equal_to(sorted_population)
+            
+            # Verify population ranges
+            assert_that(population_amount).described_as(
+                "population amounts within expected range"
+            ).all_match(lambda x: 50000 <= x <= 500000)
📝 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
assert_that(population_amount).described_as(
"population amount"
).is_equal_to([495699, 284846, 183912, 161161, 90327])
# Verify we have the expected number of cities
assert_that(population_amount).described_as(
"number of Danish cities"
).is_length(5)
# Verify population values are in descending order
sorted_population = sorted(population_amount, reverse=True)
assert_that(population_amount).described_as(
"population amounts in descending order"
).is_equal_to(sorted_population)
# Verify population ranges
assert_that(population_amount).described_as(
"population amounts within expected range"
).all_match(lambda x: 50000 <= x <= 500000)

Comment on lines +9 to +15
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Database credentials are hardcoded in the fixture

The database connection details are indeed not properly externalized. Found hardcoded credentials in tests/conftest.py:

connection = MySQLConnection(user="root", password="1234", database="world")
  • The database credentials should be moved to environment variables or a configuration file
  • Using root user with a weak password ("1234") in tests is a security concern
  • The database name "world" is also hardcoded
🔗 Analysis chain

Verify database configuration management.

The test relies on database connection, but we should ensure that connection details are properly externalized and not hardcoded in the fixture.

Let me gather more specific information about the database configuration and fixture setup.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for database configuration management

# Look for configuration files or environment variables
echo "Checking for database configuration files..."
fd -e ini -e cfg -e yaml -e env

# Check for potential hardcoded credentials
echo -e "\nChecking for potential hardcoded database credentials..."
rg -i "password|user|host|root|localhost|3306" --type py

Length of output: 7146

Script:

#!/bin/bash
# Check for database fixture and configuration setup
echo "Checking database fixture setup..."
rg -A 10 "def db_connection" --type py

echo -e "\nChecking for environment variable usage in database connection..."
rg "os\.getenv.*(?i)(db|database|sql)" --type py

echo -e "\nChecking for any config files in tests directory..."
fd . tests/ -e ini -e cfg -e yaml -e env -e json

echo -e "\nChecking for pytest configuration..."
fd pytest -e ini -e cfg

Length of output: 1131