Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds comprehensive test coverage for the Evently server application. The PR introduces unit tests for service layers and common utilities, along with code style improvements.
Key changes include:
- Addition of complete test suite for
GatheringServiceandBookingService - Unit tests for mapper extension methods
- Code formatting and import statement cleanup across TypeScript files
Reviewed Changes
Copilot reviewed 27 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Evently.Server.Test/Features/Gatherings/Services/GatheringServiceTests.cs | Comprehensive test suite for GatheringService with CRUD operations and validation |
| tests/Evently.Server.Test/Features/Bookings/Services/BookingServiceTests.cs | Unit tests for BookingService covering creation, retrieval, and updates |
| tests/Evently.Server.Test/Common/Extensions/MapperExtensionTests.cs | Tests for entity-to-DTO mapping functionality |
| tests/Evently.Server.Test/Evently.Server.Test.csproj | Test project configuration with required NuGet packages |
| Multiple TypeScript files | Import statement reorganization and code formatting improvements |
| .github/workflows/build.yml | CI/CD pipeline updated to include unit test execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| DateTimeOffset cancellation = creation.AddHours(3); | ||
|
|
||
| // Create DTO with mock values | ||
| BookingReqDto dto = new(attendeeId, bookingId, gatheringId, creation, checkIn, checkout, cancellation); |
There was a problem hiding this comment.
[nitpick] The parameter order in the BookingReqDto constructor appears inconsistent with the typical pattern where optional parameters (like cancellation) come last. The current order places attendeeId first, but typically the booking ID would be first. Consider reviewing the constructor parameter order for consistency.
| BookingReqDto dto = new(attendeeId, bookingId, gatheringId, creation, checkIn, checkout, cancellation); | |
| BookingReqDto dto = new(bookingId, attendeeId, gatheringId, creation, checkIn, checkout, cancellation); |
|
|
||
| add-migration: | ||
| dotnet ef migrations add RenameAccountId --project=src/Evently.Server --context=AppDbContext --output-dir=Common/Adapters/Data/Migrations | ||
| dotnet ef migrations add ColCollation --project=src/Evently.Server --context=AppDbContext --output-dir=Common/Adapters/Data/Migrations |
There was a problem hiding this comment.
The migration name 'ColCollation' is generic and doesn't clearly describe what the migration does. Consider using a more descriptive name that explains the specific changes being made.
| dotnet ef migrations add ColCollation --project=src/Evently.Server --context=AppDbContext --output-dir=Common/Adapters/Data/Migrations | |
| dotnet ef migrations add SetUserNameCollationToCaseInsensitive --project=src/Evently.Server --context=AppDbContext --output-dir=Common/Adapters/Data/Migrations |
No description provided.