Added unit tests for client & API#2
Merged
Merged
Conversation
Reviewer's GuideThis PR introduces comprehensive unit tests for the client and API layers, refactors module imports to a namespaced structure, updates project dependencies and CI configuration to support testing, enhances input validation in TOTPGenerator, and refines environment variable usage in Docker Compose. Class diagram for updated TOTPGenerator input validationclassDiagram
class TOTPGenerator {
+TOTP_INTERVAL_SECONDS: int
+__init__(seed_base64: str)
+generate_code() str
}
TOTPGenerator : __totp
TOTPGenerator : "raises ValueError if seed_base64 is empty"
Class diagram for namespaced module imports in APIclassDiagram
class TicketsController {
}
class UserRepository {
}
class TicketRepository {
}
class TicketService {
}
class PostgreSQLDbContext {
}
TicketsController <.. TicketService
TicketService <.. TicketRepository
TicketService <.. UserRepository
TicketRepository <.. PostgreSQLDbContext
UserRepository <.. PostgreSQLDbContext
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The tests import from src.register_ticket_api but pytest.ini sets pythonpath=src, so you should either update the imports to register_ticket_api.* or adjust pytest.ini to include the
srcpackage prefix to ensure modules resolve correctly. - The test_ticket_service file exceeds 350 lines and covers multiple scenarios—consider splitting it into smaller modules (e.g. one for register_ticket and one for log_attendance) to improve readability and maintainability.
- The mark_ticket_as_used signature currently returns Any with a comment ‘# bool’; updating the return type annotation to bool (and removing the Any) will make the API clearer and more type-safe.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests import from src.register_ticket_api but pytest.ini sets pythonpath=src, so you should either update the imports to register_ticket_api.* or adjust pytest.ini to include the `src` package prefix to ensure modules resolve correctly.
- The test_ticket_service file exceeds 350 lines and covers multiple scenarios—consider splitting it into smaller modules (e.g. one for register_ticket and one for log_attendance) to improve readability and maintainability.
- The mark_ticket_as_used signature currently returns Any with a comment ‘# bool’; updating the return type annotation to bool (and removing the Any) will make the API clearer and more type-safe.
## Individual Comments
### Comment 1
<location> `src/register_ticket_api/repositories/ticket_repository.py:56` </location>
<code_context>
return Ticket(**row)
return None
- async def mark_ticket_as_used(self, ticket_id: UUID) -> Any[bool]:
+ async def mark_ticket_as_used(self, ticket_id: UUID) -> Any: # bool
FN_NAME: str = "fn_mark_ticket_as_used"
try:
</code_context>
<issue_to_address>
**suggestion:** Return type annotation is now less specific; consider restoring type clarity.
If the function always returns a boolean, please annotate the return type as 'bool' for better maintainability and static analysis.
```suggestion
async def mark_ticket_as_used(self, ticket_id: UUID) -> bool:
```
</issue_to_address>
### Comment 2
<location> `tests/unit/register_ticket_api/services/test_ticket_service.py:339-340` </location>
<code_context>
+ )
+
+
+async def test_log_attendance_db_operation_exception(
+ ticket_service: TicketService,
+ mock_ticket_repo: AsyncMock,
+ sample_registered_ticket: Ticket,
+ sample_attendance_log: AttendanceLog,
+) -> None:
+ """Test attendance logging handles database operation errors."""
+ with patch("src.register_ticket_api.services.ticket_service.pyotp.TOTP") as mock_totp_class:
+ mock_totp_instance = MagicMock()
+ mock_totp_instance.verify.return_value = True
+ mock_totp_class.return_value = mock_totp_instance
+
+ mock_ticket_repo.get_by_ticket_details.return_value = sample_registered_ticket
+ mock_ticket_repo.mark_ticket_as_used.side_effect = DbOperationException("DB Error")
+
+ with pytest.raises(AppValidationException, match="Error creating user"):
+ await ticket_service.log_attendance(sample_attendance_log)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Error message in assertion may be misleading for attendance logging.
Please ensure the test checks for an error message relevant to attendance logging, not user creation, to accurately reflect the scenario being tested.
```suggestion
with pytest.raises(AppValidationException, match="Error logging attendance"):
await ticket_service.log_attendance(sample_attendance_log)
```
</issue_to_address>
### Comment 3
<location> `tests/unit/register_ticket_api/services/test_ticket_service.py:280-299` </location>
<code_context>
+ await ticket_service.log_attendance(sample_attendance_log)
+
+
+async def test_log_attendance_ticket_no_seed(
+ ticket_service: TicketService,
+ mock_ticket_repo: AsyncMock,
+ sample_attendance_log: AttendanceLog,
+) -> None:
+ """Test attendance logging fails when ticket has no seed."""
+ ticket_no_seed = Ticket(
+ id=uuid4(),
+ seat=TEST_SEAT,
+ gate=TEST_GATE,
+ user_id=uuid4(),
+ seed=None,
+ status="valid",
+ created_at="2024-01-01T00:00:00Z",
+ used_at=None,
+ )
+ mock_ticket_repo.get_by_ticket_details.return_value = ticket_no_seed
+
+ with pytest.raises(AppValidationException, match="Ticket has no seed"):
+ await ticket_service.log_attendance(sample_attendance_log)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Edge case for empty string seed is not covered.
Please add a test for tickets where the seed is an empty string to ensure this scenario is handled correctly.
```suggestion
async def test_log_attendance_ticket_no_seed(
ticket_service: TicketService,
mock_ticket_repo: AsyncMock,
sample_attendance_log: AttendanceLog,
) -> None:
"""Test attendance logging fails when ticket has no seed."""
ticket_no_seed = Ticket(
id=uuid4(),
seat=TEST_SEAT,
gate=TEST_GATE,
user_id=uuid4(),
seed=None,
status="valid",
created_at="2024-01-01T00:00:00Z",
used_at=None,
)
mock_ticket_repo.get_by_ticket_details.return_value = ticket_no_seed
with pytest.raises(AppValidationException, match="Ticket has no seed"):
await ticket_service.log_attendance(sample_attendance_log)
async def test_log_attendance_ticket_empty_seed(
ticket_service: TicketService,
mock_ticket_repo: AsyncMock,
sample_attendance_log: AttendanceLog,
) -> None:
"""Test attendance logging fails when ticket seed is an empty string."""
ticket_empty_seed = Ticket(
id=uuid4(),
seat=TEST_SEAT,
gate=TEST_GATE,
user_id=uuid4(),
seed="",
status="valid",
created_at="2024-01-01T00:00:00Z",
used_at=None,
)
mock_ticket_repo.get_by_ticket_details.return_value = ticket_empty_seed
with pytest.raises(AppValidationException, match="Ticket has no seed"):
await ticket_service.log_attendance(sample_attendance_log)
```
</issue_to_address>
### Comment 4
<location> `tests/unit/register_ticket_api/repositories/test_ticket_repository.py:39-59` </location>
<code_context>
+ return TicketRepository(db_context=mock_db_context)
+
+
+async def test_register_ticket_calls_stored_procedure(
+ mock_db_context: AsyncMock,
+ sample_user: User,
+ sample_ticket: Ticket,
+ ticket_repository: TicketRepository,
+) -> None:
+ mock_conn = AsyncMock()
+ mock_conn.execute.return_value = 1 # emulates 1 affected row
+ mock_db_context.get_connection.return_value = mock_conn
+
+ result: bool = await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket)
+
+ mock_conn.execute.assert_awaited_once_with(
+ "CALL sp_register_ticket_to_user($1, $2)",
+ sample_ticket.id,
+ sample_user.id,
+ )
+ assert result is True
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for register_ticket when the stored procedure fails.
Please add a test case for register_ticket that covers stored procedure failure scenarios, such as returning 0 affected rows or raising an exception, to verify proper error handling.
```suggestion
+
+async def test_register_ticket_calls_stored_procedure(
+ mock_db_context: AsyncMock,
+ sample_user: User,
+ sample_ticket: Ticket,
+ ticket_repository: TicketRepository,
+) -> None:
+ mock_conn = AsyncMock()
+ mock_conn.execute.return_value = 1 # emulates 1 affected row
+ mock_db_context.get_connection.return_value = mock_conn
+
+ result: bool = await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket)
+
+ mock_conn.execute.assert_awaited_once_with(
+ "CALL sp_register_ticket_to_user($1, $2)",
+ sample_ticket.id,
+ sample_user.id,
+ )
+ assert result is True
+
+
+# Test when stored procedure returns 0 affected rows (failure)
+async def test_register_ticket_returns_false_on_zero_rows(
+ mock_db_context: AsyncMock,
+ sample_user: User,
+ sample_ticket: Ticket,
+ ticket_repository: TicketRepository,
+) -> None:
+ mock_conn = AsyncMock()
+ mock_conn.execute.return_value = 0 # emulates 0 affected rows
+ mock_db_context.get_connection.return_value = mock_conn
+
+ result: bool = await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket)
+ assert result is False
+
+
+# Test when stored procedure raises an exception
+async def test_register_ticket_raises_on_db_exception(
+ mock_db_context: AsyncMock,
+ sample_user: User,
+ sample_ticket: Ticket,
+ ticket_repository: TicketRepository,
+) -> None:
+ mock_conn = AsyncMock()
+ mock_conn.execute.side_effect = Exception("DB error")
+ mock_db_context.get_connection.return_value = mock_conn
+
+ with pytest.raises(DbOperationException):
+ await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket)
+
```
</issue_to_address>
### Comment 5
<location> `tests/unit/register_ticket_api/repositories/test_ticket_repository.py:57` </location>
<code_context>
async def test_register_ticket_calls_stored_procedure(
mock_db_context: AsyncMock,
sample_user: User,
sample_ticket: Ticket,
ticket_repository: TicketRepository,
) -> None:
mock_conn = AsyncMock()
mock_conn.execute.return_value = 1 # emulates 1 affected row
mock_db_context.get_connection.return_value = mock_conn
result: bool = await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket)
mock_conn.execute.assert_awaited_once_with(
"CALL sp_register_ticket_to_user($1, $2)",
sample_ticket.id,
sample_user.id,
)
assert result is True
</code_context>
<issue_to_address>
**suggestion (code-quality):** Simplify comparison to boolean ([`simplify-boolean-comparison`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/simplify-boolean-comparison/))
```suggestion
assert result
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return Ticket(**row) | ||
| return None | ||
|
|
||
| async def mark_ticket_as_used(self, ticket_id: UUID) -> Any[bool]: |
There was a problem hiding this comment.
suggestion: Return type annotation is now less specific; consider restoring type clarity.
If the function always returns a boolean, please annotate the return type as 'bool' for better maintainability and static analysis.
Suggested change
| async def mark_ticket_as_used(self, ticket_id: UUID) -> Any[bool]: | |
| async def mark_ticket_as_used(self, ticket_id: UUID) -> bool: |
Comment on lines
+339
to
+340
| with pytest.raises(AppValidationException, match="Error creating user"): | ||
| await ticket_service.log_attendance(sample_attendance_log) |
There was a problem hiding this comment.
suggestion (testing): Error message in assertion may be misleading for attendance logging.
Please ensure the test checks for an error message relevant to attendance logging, not user creation, to accurately reflect the scenario being tested.
Suggested change
| with pytest.raises(AppValidationException, match="Error creating user"): | |
| await ticket_service.log_attendance(sample_attendance_log) | |
| with pytest.raises(AppValidationException, match="Error logging attendance"): | |
| await ticket_service.log_attendance(sample_attendance_log) |
Comment on lines
+280
to
+299
| async def test_log_attendance_ticket_no_seed( | ||
| ticket_service: TicketService, | ||
| mock_ticket_repo: AsyncMock, | ||
| sample_attendance_log: AttendanceLog, | ||
| ) -> None: | ||
| """Test attendance logging fails when ticket has no seed.""" | ||
| ticket_no_seed = Ticket( | ||
| id=uuid4(), | ||
| seat=TEST_SEAT, | ||
| gate=TEST_GATE, | ||
| user_id=uuid4(), | ||
| seed=None, | ||
| status="valid", | ||
| created_at="2024-01-01T00:00:00Z", | ||
| used_at=None, | ||
| ) | ||
| mock_ticket_repo.get_by_ticket_details.return_value = ticket_no_seed | ||
|
|
||
| with pytest.raises(AppValidationException, match="Ticket has no seed"): | ||
| await ticket_service.log_attendance(sample_attendance_log) |
There was a problem hiding this comment.
suggestion (testing): Edge case for empty string seed is not covered.
Please add a test for tickets where the seed is an empty string to ensure this scenario is handled correctly.
Suggested change
| async def test_log_attendance_ticket_no_seed( | |
| ticket_service: TicketService, | |
| mock_ticket_repo: AsyncMock, | |
| sample_attendance_log: AttendanceLog, | |
| ) -> None: | |
| """Test attendance logging fails when ticket has no seed.""" | |
| ticket_no_seed = Ticket( | |
| id=uuid4(), | |
| seat=TEST_SEAT, | |
| gate=TEST_GATE, | |
| user_id=uuid4(), | |
| seed=None, | |
| status="valid", | |
| created_at="2024-01-01T00:00:00Z", | |
| used_at=None, | |
| ) | |
| mock_ticket_repo.get_by_ticket_details.return_value = ticket_no_seed | |
| with pytest.raises(AppValidationException, match="Ticket has no seed"): | |
| await ticket_service.log_attendance(sample_attendance_log) | |
| async def test_log_attendance_ticket_no_seed( | |
| ticket_service: TicketService, | |
| mock_ticket_repo: AsyncMock, | |
| sample_attendance_log: AttendanceLog, | |
| ) -> None: | |
| """Test attendance logging fails when ticket has no seed.""" | |
| ticket_no_seed = Ticket( | |
| id=uuid4(), | |
| seat=TEST_SEAT, | |
| gate=TEST_GATE, | |
| user_id=uuid4(), | |
| seed=None, | |
| status="valid", | |
| created_at="2024-01-01T00:00:00Z", | |
| used_at=None, | |
| ) | |
| mock_ticket_repo.get_by_ticket_details.return_value = ticket_no_seed | |
| with pytest.raises(AppValidationException, match="Ticket has no seed"): | |
| await ticket_service.log_attendance(sample_attendance_log) | |
| async def test_log_attendance_ticket_empty_seed( | |
| ticket_service: TicketService, | |
| mock_ticket_repo: AsyncMock, | |
| sample_attendance_log: AttendanceLog, | |
| ) -> None: | |
| """Test attendance logging fails when ticket seed is an empty string.""" | |
| ticket_empty_seed = Ticket( | |
| id=uuid4(), | |
| seat=TEST_SEAT, | |
| gate=TEST_GATE, | |
| user_id=uuid4(), | |
| seed="", | |
| status="valid", | |
| created_at="2024-01-01T00:00:00Z", | |
| used_at=None, | |
| ) | |
| mock_ticket_repo.get_by_ticket_details.return_value = ticket_empty_seed | |
| with pytest.raises(AppValidationException, match="Ticket has no seed"): | |
| await ticket_service.log_attendance(sample_attendance_log) |
Comment on lines
+39
to
+59
|
|
||
| async def test_register_ticket_calls_stored_procedure( | ||
| mock_db_context: AsyncMock, | ||
| sample_user: User, | ||
| sample_ticket: Ticket, | ||
| ticket_repository: TicketRepository, | ||
| ) -> None: | ||
| mock_conn = AsyncMock() | ||
| mock_conn.execute.return_value = 1 # emulates 1 affected row | ||
| mock_db_context.get_connection.return_value = mock_conn | ||
|
|
||
| result: bool = await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket) | ||
|
|
||
| mock_conn.execute.assert_awaited_once_with( | ||
| "CALL sp_register_ticket_to_user($1, $2)", | ||
| sample_ticket.id, | ||
| sample_user.id, | ||
| ) | ||
| assert result is True | ||
|
|
||
|
|
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for register_ticket when the stored procedure fails.
Please add a test case for register_ticket that covers stored procedure failure scenarios, such as returning 0 affected rows or raising an exception, to verify proper error handling.
Suggested change
| async def test_register_ticket_calls_stored_procedure( | |
| mock_db_context: AsyncMock, | |
| sample_user: User, | |
| sample_ticket: Ticket, | |
| ticket_repository: TicketRepository, | |
| ) -> None: | |
| mock_conn = AsyncMock() | |
| mock_conn.execute.return_value = 1 # emulates 1 affected row | |
| mock_db_context.get_connection.return_value = mock_conn | |
| result: bool = await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket) | |
| mock_conn.execute.assert_awaited_once_with( | |
| "CALL sp_register_ticket_to_user($1, $2)", | |
| sample_ticket.id, | |
| sample_user.id, | |
| ) | |
| assert result is True | |
| + | |
| +async def test_register_ticket_calls_stored_procedure( | |
| + mock_db_context: AsyncMock, | |
| + sample_user: User, | |
| + sample_ticket: Ticket, | |
| + ticket_repository: TicketRepository, | |
| +) -> None: | |
| + mock_conn = AsyncMock() | |
| + mock_conn.execute.return_value = 1 # emulates 1 affected row | |
| + mock_db_context.get_connection.return_value = mock_conn | |
| + | |
| + result: bool = await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket) | |
| + | |
| + mock_conn.execute.assert_awaited_once_with( | |
| + "CALL sp_register_ticket_to_user($1, $2)", | |
| + sample_ticket.id, | |
| + sample_user.id, | |
| + ) | |
| + assert result is True | |
| + | |
| + | |
| +# Test when stored procedure returns 0 affected rows (failure) | |
| +async def test_register_ticket_returns_false_on_zero_rows( | |
| + mock_db_context: AsyncMock, | |
| + sample_user: User, | |
| + sample_ticket: Ticket, | |
| + ticket_repository: TicketRepository, | |
| +) -> None: | |
| + mock_conn = AsyncMock() | |
| + mock_conn.execute.return_value = 0 # emulates 0 affected rows | |
| + mock_db_context.get_connection.return_value = mock_conn | |
| + | |
| + result: bool = await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket) | |
| + assert result is False | |
| + | |
| + | |
| +# Test when stored procedure raises an exception | |
| +async def test_register_ticket_raises_on_db_exception( | |
| + mock_db_context: AsyncMock, | |
| + sample_user: User, | |
| + sample_ticket: Ticket, | |
| + ticket_repository: TicketRepository, | |
| +) -> None: | |
| + mock_conn = AsyncMock() | |
| + mock_conn.execute.side_effect = Exception("DB error") | |
| + mock_db_context.get_connection.return_value = mock_conn | |
| + | |
| + with pytest.raises(DbOperationException): | |
| + await ticket_repository.register_ticket(user=sample_user, ticket=sample_ticket) | |
| + |
| sample_ticket.id, | ||
| sample_user.id, | ||
| ) | ||
| assert result is True |
There was a problem hiding this comment.
suggestion (code-quality): Simplify comparison to boolean (simplify-boolean-comparison)
Suggested change
| assert result is True | |
| assert result |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a comprehensive unit test suite for client and API components, refine module imports and Docker Compose configuration, and enhance CI pipeline and development dependencies to support testing
Enhancements:
Build:
CI:
Documentation:
Tests: