Skip to content

Conversation

@Tr1sma
Copy link
Owner

@Tr1sma Tr1sma commented Jan 5, 2026

No description provided.

Copilot AI review requested due to automatic review settings January 5, 2026 23:16
@Tr1sma Tr1sma merged commit d8d2c24 into main Jan 5, 2026
1 check passed
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds comprehensive unit test coverage for ViewModels and Validators, includes a GenreService test suite, and fixes missing SaveChanges calls in the GenreService. However, the PR contains build errors that prevent successful compilation and includes a build log file that should not be committed.

Key changes:

  • Added 4 new ViewModel test files covering Stats, Reading, Dashboard, and BookEdit ViewModels using NSubstitute for mocking
  • Added 3 new Validator test files for Book, ReadingSession, and ReadingGoal using FluentValidation.TestHelper
  • Added comprehensive GenreService tests with cache mocking
  • Fixed GenreService by adding missing SaveChangesAsync calls after Add, Update, Delete, and relationship operations
  • Added NSubstitute 5.3.0 package reference for mocking support

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
build_log.txt Build output log containing compilation errors - should not be committed to version control
BookLoggerApp.Tests/Unit/ViewModels/StatsViewModelTests.cs Tests for StatsViewModel covering statistics loading, level progression calculation, and top book filtering
BookLoggerApp.Tests/Unit/ViewModels/ReadingViewModelTests.cs Tests for ReadingViewModel covering session start, pause/resume, page updates, and session completion
BookLoggerApp.Tests/Unit/ViewModels/DashboardViewModelTests.cs Tests for DashboardViewModel covering dashboard data loading and plant watering functionality
BookLoggerApp.Tests/Unit/ViewModels/BookEditViewModelTests.cs Tests for BookEditViewModel covering book creation, editing, validation, ISBN lookup, and deletion
BookLoggerApp.Tests/Unit/Validators/ReadingSessionValidatorTests.cs Comprehensive validation tests for ReadingSession model covering all validation rules
BookLoggerApp.Tests/Unit/Validators/ReadingGoalValidatorTests.cs Comprehensive validation tests for ReadingGoal model covering all validation rules
BookLoggerApp.Tests/Unit/Validators/BookValidatorTests.cs Comprehensive validation tests for Book model covering all validation rules
BookLoggerApp.Tests/Unit/Services/GenreServiceTests.cs Tests for GenreService covering CRUD operations, caching behavior, and genre-book relationships
BookLoggerApp.Tests/BookLoggerApp.Tests.csproj Added NSubstitute 5.3.0 package reference for mocking framework support
BookLoggerApp.Infrastructure/Services/GenreService.cs Fixed bug by adding missing SaveChangesAsync calls in Add, Update, Delete, AddGenreToBook, and RemoveGenreFromBook methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +71 to +76
var settings = new AppSettings { UserLevel = 2, TotalXp = 175 }; // Level 1 is 100xp, so 75xp into Level 2 (which needs 150xp total delta? No, formula is different)
// Formula: Level 1 = 100 XP. Level 2 req 150 XP. Cumulative: L1=100.
// wait, source code: GetXpForLevel(1) = 100. xpForPreviousLevels(level 2) loops i=1 to 1. sums GetXpForLevel(1) = 100.
// CurrentLevelXp = TotalXp (175) - 100 = 75.
// NextLevelXp = GetXpForLevel(2) = 100 * 1.5^1 = 150.
// Percentage = 75 / 150 = 50%
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The comment contains verbose working-through-the-logic text that should be cleaned up. Comments in tests should explain what is being tested and why, not show the mental process of figuring out the calculation. Consider condensing this to a clear explanation of the expected behavior, for example: "Level 2 requires 150 XP total. With 175 total XP, user has 75 XP into level 2 (175 - 100 from level 1 = 75). Progress percentage should be 75/150 = 50%."

Suggested change
var settings = new AppSettings { UserLevel = 2, TotalXp = 175 }; // Level 1 is 100xp, so 75xp into Level 2 (which needs 150xp total delta? No, formula is different)
// Formula: Level 1 = 100 XP. Level 2 req 150 XP. Cumulative: L1=100.
// wait, source code: GetXpForLevel(1) = 100. xpForPreviousLevels(level 2) loops i=1 to 1. sums GetXpForLevel(1) = 100.
// CurrentLevelXp = TotalXp (175) - 100 = 75.
// NextLevelXp = GetXpForLevel(2) = 100 * 1.5^1 = 150.
// Percentage = 75 / 150 = 50%
var settings = new AppSettings { UserLevel = 2, TotalXp = 175 };
// Level 1 requires 100 XP and level 2 requires 150 XP. With 175 total XP, the user has
// 75 XP into level 2 (175 - 100), so progress should be 75 / 150 = 50%.

Copilot uses AI. Check for mistakes.
Comment on lines +86 to +89
// Check session update
// Use Arg.Any or simply verify call happened, as object reference is mutable and NSubstitute captures reference
await _progressService.Received().UpdateSessionAsync(session);
// Also verify property values on the session object invoked
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The comment is unnecessarily verbose and could be condensed. Consider simplifying to just explain the verification approach being used rather than including implementation details about NSubstitute's capture mechanism.

Suggested change
// Check session update
// Use Arg.Any or simply verify call happened, as object reference is mutable and NSubstitute captures reference
await _progressService.Received().UpdateSessionAsync(session);
// Also verify property values on the session object invoked
// Verify the session was updated
await _progressService.Received().UpdateSessionAsync(session);
// Verify the updated session values

Copilot uses AI. Check for mistakes.
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.

2 participants