Refactor services for testability and clean up obsolete tests #2

punkouter26 · 2026-01-29T15:35:20Z

This PR addresses issues in the test suite and improves code quality.

Environment Fix: Updated global.json to use .NET SDK 10.0.100.
Test Cleanup: Deleted LyricsControllerTests.cs, TranslationControllerTests.cs, and ApiResponseTimeMiddlewareTests.cs which contained dead code/TODOs for removed components.
Refactoring: Updated TranslationService to support constructor injection of ChatClient, enabling unit testing without external dependencies.
Test Coverage:
- Added TranslationServiceTests covering success, rate-limiting, and error scenarios.
- Added LyricsServiceTests covering data loading and retrieval logic.
- Verified all unit tests pass.

PR created automatically by Jules for task 4888884607815198208 started by @punkouter26

…ve obsolete tests - Refactored TranslationService to accept optional ChatClient for DI/testing. - Added comprehensive unit tests for TranslationService (mocking OpenAI client). - Added unit tests for LyricsService (mocking file system access). - Removed obsolete/commented-out test files for removed Controllers and Middleware. - Fixed global.json to match installed SDK version. Co-authored-by: punkouter26 <[email protected]>

google-labs-jules · 2026-01-29T15:35:21Z

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

For security, I will only act on instructions from the user who triggered this task.

codereviewbot-ai · 2026-01-29T15:35:57Z

src/PoVicTranslate.Web/Services/TranslationService.cs

        TelemetryClient telemetryClient,
-        ILogger<TranslationService> logger)
+        ILogger<TranslationService> logger,


Potential NullReferenceException:
The constructor does not validate telemetryClient and logger for null. If either is not provided, subsequent usage will result in a runtime exception.

Recommended solution:
Add explicit null checks for these dependencies:

ArgumentNullException.ThrowIfNull(telemetryClient); ArgumentNullException.ThrowIfNull(logger);

codereviewbot-ai · 2026-01-29T15:35:57Z

src/PoVicTranslate.Web/Services/TranslationService.cs

        _deploymentName = settings.AzureOpenAIDeploymentName;

+        if (chatClient != null)
+        {
+            _chatClient = chatClient;
+            return;
+        }
+
        if (string.IsNullOrWhiteSpace(settings.AzureOpenAIApiKey) ||
            string.IsNullOrWhiteSpace(settings.AzureOpenAIEndpoint) ||
            string.IsNullOrWhiteSpace(settings.AzureOpenAIDeploymentName))


Inconsistent Configuration Validation:
The validation of Azure OpenAI settings (lines 49-51) is only performed if chatClient is null. However, _deploymentName is always set from settings.AzureOpenAIDeploymentName (line 41), even when chatClient is provided. If this value is null or whitespace, it could cause runtime errors later.

Recommended solution:
Validate settings.AzureOpenAIDeploymentName for null or whitespace regardless of whether chatClient is provided.

codereviewbot-ai · 2026-01-29T15:35:57Z

tests/Po.VicTranslate.UnitTests/Services/Lyrics/LyricsServiceTests.cs

+    {
+        if (Directory.Exists(_tempDirectory))
+        {
+            try { Directory.Delete(_tempDirectory, true); } catch { }


The catch block in Dispose silently swallows all exceptions during directory deletion:

try { Directory.Delete(_tempDirectory, true); } catch { }

This can mask underlying issues with test cleanup, such as file locks or permission errors, making debugging more difficult. Consider logging the exception or rethrowing it to ensure test failures are visible if cleanup does not succeed.

codereviewbot-ai · 2026-01-29T15:35:57Z

tests/Po.VicTranslate.UnitTests/Services/TranslationServiceTests.cs

+    [Fact]
+    public async Task TranslateToVictorianEnglishAsync_WithValidText_ShouldReturnTranslation()
+    {
+        // Arrange
+        var mockChatClient = new Mock<ChatClient>("gpt-4o", new ApiKeyCredential("key"), new OpenAIClientOptions());
+        var expectedTranslation = "Good morrow.";
+
+        var content = new ChatMessageContent(expectedTranslation);
+        var chatCompletion = OpenAIChatModelFactory.ChatCompletion(content: content);
+
+        var mockResponse = new Mock<PipelineResponse>();
+        mockResponse.Setup(r => r.Status).Returns(200);
+
+        var clientResult = ClientResult.FromValue(chatCompletion, mockResponse.Object);
+
+        mockChatClient.Setup(x => x.CompleteChatAsync(
+            It.IsAny<ChatMessage[]>()))
+            .ReturnsAsync(clientResult);
+
+        var service = new TranslationService(
+            _mockOptions.Object,
+            _telemetryClient,
+            _mockLogger.Object,
+            mockChatClient.Object);
+
+        // Act
+        var result = await service.TranslateToVictorianEnglishAsync("Hello");
+
+        // Assert
+        result.Should().Be(expectedTranslation);
+    }
+
+    [Fact]
+    public async Task TranslateToVictorianEnglishAsync_WhenRateLimited_ShouldReturnPoliteError()
+    {
+        // Arrange
+        var mockChatClient = new Mock<ChatClient>("gpt-4o", new ApiKeyCredential("key"), new OpenAIClientOptions());
+
+        var mockResponse = new Mock<PipelineResponse>();
+        mockResponse.Setup(r => r.Status).Returns(429);
+
+        var exception = new ClientResultException(mockResponse.Object);
+
+        mockChatClient.Setup(x => x.CompleteChatAsync(
+            It.IsAny<ChatMessage[]>()))
+            .ThrowsAsync(exception);
+
+        var service = new TranslationService(
+            _mockOptions.Object,
+            _telemetryClient,
+            _mockLogger.Object,
+            mockChatClient.Object);
+
+        // Act
+        var result = await service.TranslateToVictorianEnglishAsync("Hello");
+
+        // Assert
+        result.Should().Contain("Alas, our translation apparatus finds itself most overwhelmed");
+    }
+
+    [Fact]
+    public async Task TranslateToVictorianEnglishAsync_WhenErrorOccurs_ShouldReturnErrorMessage()
+    {
+        // Arrange
+        var mockChatClient = new Mock<ChatClient>("gpt-4o", new ApiKeyCredential("key"), new OpenAIClientOptions());
+
+        mockChatClient.Setup(x => x.CompleteChatAsync(
+            It.IsAny<ChatMessage[]>()))
+            .ThrowsAsync(new InvalidOperationException("Generic error"));
+
+        var service = new TranslationService(
+            _mockOptions.Object,
+            _telemetryClient,
+            _mockLogger.Object,
+            mockChatClient.Object);
+
+        // Act
+        var result = await service.TranslateToVictorianEnglishAsync("Hello");
+
+        // Assert
+        result.Should().Contain("Regrettably, an unforeseen circumstance");
+    }


The current tests for TranslateToVictorianEnglishAsync do not cover the scenario where the input text is null or empty. This is a critical edge case that should be validated to ensure the service handles such input gracefully and does not throw unexpected exceptions or return misleading results.

Recommendation:
Add a test case similar to the following:

[Theory] [InlineData(null)] [InlineData("")] public async Task TranslateToVictorianEnglishAsync_WithNullOrEmptyInput_ShouldReturnErrorMessage(string? input) { var service = new TranslationService(_mockOptions.Object, _telemetryClient, _mockLogger.Object); var result = await service.TranslateToVictorianEnglishAsync(input); result.Should().Contain("input cannot be empty"); // Adjust expected message as appropriate }

This will improve robustness and input validation coverage.

codereviewbot-ai · 2026-01-29T15:35:58Z

tests/Po.VicTranslate.UnitTests/Services/TranslationServiceTests.cs

+    [Fact]
+    public async Task TranslateToVictorianEnglishAsync_WhenRateLimited_ShouldReturnPoliteError()
+    {
+        // Arrange
+        var mockChatClient = new Mock<ChatClient>("gpt-4o", new ApiKeyCredential("key"), new OpenAIClientOptions());
+
+        var mockResponse = new Mock<PipelineResponse>();
+        mockResponse.Setup(r => r.Status).Returns(429);
+
+        var exception = new ClientResultException(mockResponse.Object);
+
+        mockChatClient.Setup(x => x.CompleteChatAsync(
+            It.IsAny<ChatMessage[]>()))
+            .ThrowsAsync(exception);
+
+        var service = new TranslationService(
+            _mockOptions.Object,
+            _telemetryClient,
+            _mockLogger.Object,
+            mockChatClient.Object);
+
+        // Act
+        var result = await service.TranslateToVictorianEnglishAsync("Hello");
+
+        // Assert
+        result.Should().Contain("Alas, our translation apparatus finds itself most overwhelmed");
+    }
+
+    [Fact]
+    public async Task TranslateToVictorianEnglishAsync_WhenErrorOccurs_ShouldReturnErrorMessage()
+    {
+        // Arrange
+        var mockChatClient = new Mock<ChatClient>("gpt-4o", new ApiKeyCredential("key"), new OpenAIClientOptions());
+
+        mockChatClient.Setup(x => x.CompleteChatAsync(
+            It.IsAny<ChatMessage[]>()))
+            .ThrowsAsync(new InvalidOperationException("Generic error"));
+
+        var service = new TranslationService(
+            _mockOptions.Object,
+            _telemetryClient,
+            _mockLogger.Object,
+            mockChatClient.Object);
+
+        // Act
+        var result = await service.TranslateToVictorianEnglishAsync("Hello");
+
+        // Assert
+        result.Should().Contain("Regrettably, an unforeseen circumstance");
+    }


The error handling tests (rate limiting and generic error) do not verify that the error conditions are logged as expected. Logging is crucial for diagnosing issues in production environments.

Recommendation:
Enhance these tests to verify that the logger is called with the appropriate error messages when exceptions occur. For example:

_mockLogger.Verify( x => x.Log( LogLevel.Error, It.IsAny<EventId>(), It.Is<It.IsAnyType>((v, t) => v.ToString()!.Contains("Rate limit exceeded")), It.IsAny<Exception>(), It.IsAny<Func<It.IsAnyType, Exception?, string>>()), Times.Once);

This will ensure that error scenarios are observable and maintainable.

Copilot

Pull request overview

Refactors TranslationService for improved testability and cleans up the unit test suite by removing obsolete tests and adding new service-level coverage.

Changes:

Updated TranslationService to allow constructor injection of ChatClient (enabling unit tests without real Azure OpenAI dependencies).
Added unit tests for TranslationService and introduced new unit tests for LyricsService.
Removed obsolete/dead test files for removed controllers/middleware and adjusted SDK pinning in global.json.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/Po.VicTranslate.UnitTests/Services/TranslationServiceTests.cs	Adds unit coverage for translation success/rate-limit/error using an injected/mocked `ChatClient`.
tests/Po.VicTranslate.UnitTests/Services/Lyrics/LyricsServiceTests.cs	Introduces unit tests for song listing, lyric retrieval, and truncation behavior.
tests/Po.VicTranslate.UnitTests/Middleware/ApiResponseTimeMiddlewareTests.cs	Removes obsolete middleware tests for a component that no longer exists.
tests/Po.VicTranslate.UnitTests/Controllers/TranslationControllerTests.cs	Removes obsolete controller tests after migration to minimal APIs.
tests/Po.VicTranslate.UnitTests/Controllers/LyricsControllerTests.cs	Removes obsolete controller tests after migration to minimal APIs.
src/PoVicTranslate.Web/Services/TranslationService.cs	Enables constructor injection of `ChatClient` to decouple from external Azure OpenAI client creation.
global.json	Pins the SDK version to 10.0.100 (with rollForward to latestPatch).

Copilot · 2026-01-29T15:44:59Z

tests/Po.VicTranslate.UnitTests/Services/Lyrics/LyricsServiceTests.cs

+        // MaxWords is 200 + "..."
+        // The service logic: string.Join(" ", words.Take(MaxWords)) + "...";
+        var parts = lyrics!.Split(' ');
+        // The last part is "...". But wait, the service does: string.Join(" ", ...) + "..."
+        // So it appends "..." to the string. It doesn't mean the last word IS "...".
+        // It appends it directly. So "word199..." if no space?
+        // Code: string.Join(" ", words.Take(MaxWords)) + "...";
+        // Yes, likely "word199..." if joined by space.
+
+        // Just verify length is less than full length.


parts is declared but never used. With TreatWarningsAsErrors enabled, this will likely fail the build due to an unused-variable diagnostic. Remove it, or use it to assert something meaningful (e.g., verify the truncated output contains exactly the expected number of words / words+suffix).

Suggested change

// MaxWords is 200 + "..."

// The service logic: string.Join(" ", words.Take(MaxWords)) + "...";

var parts = lyrics!.Split(' ');

// The last part is "...". But wait, the service does: string.Join(" ", ...) + "..."

// So it appends "..." to the string. It doesn't mean the last word IS "...".

// It appends it directly. So "word199..." if no space?

// Code: string.Join(" ", words.Take(MaxWords)) + "...";

// Yes, likely "word199..." if joined by space.

// Just verify length is less than full length.

// MaxWords is 200, and the service does: string.Join(" ", words.Take(MaxWords)) + "...";

var parts = lyrics!.Split(' ');

parts.Length.Should().Be(200);

parts[parts.Length - 1].EndsWith("...").Should().BeTrue();

// Verify the truncated lyrics are shorter than the full lyrics.

Copilot · 2026-01-29T15:45:00Z

tests/Po.VicTranslate.UnitTests/Services/Lyrics/LyricsServiceTests.cs

+    private void CreateLyricsFile(LyricsCollection collection)
+    {
+        var json = JsonSerializer.Serialize(collection);
+        File.WriteAllText(_lyricsFilePath, json);
+    }


These tests serialize LyricsCollection/SongEntry to create the test lyrics-collection.json, but the checked-in runtime data file src/PoVicTranslate.Web/Data/lyrics-collection.json appears to use a different schema (e.g., top-level songs and per-song content). As a result, the tests may pass while not validating the real deserialization path used in production. Consider aligning the test JSON with the actual file format (or updating the service/models to match the file) so the tests exercise the same contract the app relies on.

Copilot · 2026-01-29T15:45:00Z

tests/Po.VicTranslate.UnitTests/Services/Lyrics/LyricsServiceTests.cs

+    {
+        if (Directory.Exists(_tempDirectory))
+        {
+            try { Directory.Delete(_tempDirectory, true); } catch { }


Poor error handling: empty catch block.

Copilot AI review requested due to automatic review settings January 29, 2026 15:35

Copilot started reviewing on behalf of punkouter26 January 29, 2026 15:35 View session

codereviewbot-ai bot reviewed Jan 29, 2026

View reviewed changes

Copilot AI reviewed Jan 29, 2026

View reviewed changes

punkouter26 closed this Feb 3, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor services for testability and clean up obsolete tests #2

Refactor services for testability and clean up obsolete tests #2

Uh oh!

punkouter26 commented Jan 29, 2026

Uh oh!

google-labs-jules bot commented Jan 29, 2026

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 29, 2026

Uh oh!

Copilot AI Jan 29, 2026

Uh oh!

Copilot AI Jan 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Refactor services for testability and clean up obsolete tests #2

Refactor services for testability and clean up obsolete tests #2

Uh oh!

Conversation

punkouter26 commented Jan 29, 2026

Uh oh!

google-labs-jules bot commented Jan 29, 2026

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants