From c48398d19634194ab5211de4c9b825a0cd6f1c79 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 29 Jan 2026 15:35:17 +0000 Subject: [PATCH] Refactor TranslationService for testability, add unit tests, and remove 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 <121304072+punkouter26@users.noreply.github.com> --- global.json | 2 +- .../Services/TranslationService.cs | 9 +- .../Controllers/LyricsControllerTests.cs | 74 ------ .../Controllers/TranslationControllerTests.cs | 60 ----- .../ApiResponseTimeMiddlewareTests.cs | 221 ------------------ .../Services/Lyrics/LyricsServiceTests.cs | 148 ++++++++++++ .../Services/TranslationServiceTests.cs | 88 +++++++ 7 files changed, 245 insertions(+), 357 deletions(-) delete mode 100644 tests/Po.VicTranslate.UnitTests/Controllers/LyricsControllerTests.cs delete mode 100644 tests/Po.VicTranslate.UnitTests/Controllers/TranslationControllerTests.cs delete mode 100644 tests/Po.VicTranslate.UnitTests/Middleware/ApiResponseTimeMiddlewareTests.cs create mode 100644 tests/Po.VicTranslate.UnitTests/Services/Lyrics/LyricsServiceTests.cs diff --git a/global.json b/global.json index 570baa6..cca5e9a 100644 --- a/global.json +++ b/global.json @@ -1,6 +1,6 @@ { "sdk": { - "version": "10.0.102", + "version": "10.0.100", "rollForward": "latestPatch", "allowPrerelease": false } diff --git a/src/PoVicTranslate.Web/Services/TranslationService.cs b/src/PoVicTranslate.Web/Services/TranslationService.cs index 8f7920e..5480b30 100644 --- a/src/PoVicTranslate.Web/Services/TranslationService.cs +++ b/src/PoVicTranslate.Web/Services/TranslationService.cs @@ -30,7 +30,8 @@ Maintain the sentiment and intent of the original text. public TranslationService( IOptions apiSettings, TelemetryClient telemetryClient, - ILogger logger) + ILogger logger, + ChatClient? chatClient = null) { ArgumentNullException.ThrowIfNull(apiSettings); _telemetryClient = telemetryClient; @@ -39,6 +40,12 @@ public TranslationService( var settings = apiSettings.Value; _deploymentName = settings.AzureOpenAIDeploymentName; + if (chatClient != null) + { + _chatClient = chatClient; + return; + } + if (string.IsNullOrWhiteSpace(settings.AzureOpenAIApiKey) || string.IsNullOrWhiteSpace(settings.AzureOpenAIEndpoint) || string.IsNullOrWhiteSpace(settings.AzureOpenAIDeploymentName)) diff --git a/tests/Po.VicTranslate.UnitTests/Controllers/LyricsControllerTests.cs b/tests/Po.VicTranslate.UnitTests/Controllers/LyricsControllerTests.cs deleted file mode 100644 index 08e8c7f..0000000 --- a/tests/Po.VicTranslate.UnitTests/Controllers/LyricsControllerTests.cs +++ /dev/null @@ -1,74 +0,0 @@ -// TODO: These tests were for LyricsController which has been converted to Minimal API endpoints (LyricsEndpoints). -// The endpoint methods are now static extension methods and need different testing approaches. -// Consider testing the ILyricsService directly or using integration tests for the endpoints. - -/* -using FluentAssertions; -using Microsoft.AspNetCore.Mvc; -using Moq; -using PoVicTranslate.Web.Endpoints; // Note: Endpoints are now static extension classes -using PoVicTranslate.Web.Services; -using Xunit; - -namespace Po.VicTranslate.UnitTests.Controllers; - -public class LyricsControllerTests -{ - private readonly Mock _mockLyricsService; - // LyricsController no longer exists - converted to LyricsEndpoints (Minimal API) - - public LyricsControllerTests() - { - _mockLyricsService = new Mock(); - } - - [Fact] - public async Task GetAvailableSongs_ShouldReturnSongList() - { - // Arrange - var songIds = new List { "song1", "song2" }; - - _mockLyricsService - .Setup(x => x.GetAvailableSongsAsync()) - .ReturnsAsync(songIds); - - // Act - // TODO: Test via integration tests or test ILyricsService directly - - // Assert - } - - [Fact] - public async Task GetLyrics_WithValidId_ShouldReturnLyrics() - { - // Arrange - const string songFileName = "test-song"; - const string lyrics = "Test lyrics content with many words to test limiting functionality here"; - - _mockLyricsService - .Setup(x => x.GetLyricsAsync(songFileName)) - .ReturnsAsync(lyrics); - - // Act - // TODO: Test via integration tests or test ILyricsService directly - - // Assert - } - - [Fact] - public async Task GetLyrics_WithInvalidId_ShouldReturnNotFound() - { - // Arrange - const string songFileName = "invalid-song"; - - _mockLyricsService - .Setup(x => x.GetLyricsAsync(songFileName)) - .ReturnsAsync((string)null!); - - // Act - // TODO: Test via integration tests or test ILyricsService directly - - // Assert - } -} -*/ diff --git a/tests/Po.VicTranslate.UnitTests/Controllers/TranslationControllerTests.cs b/tests/Po.VicTranslate.UnitTests/Controllers/TranslationControllerTests.cs deleted file mode 100644 index 9d7a256..0000000 --- a/tests/Po.VicTranslate.UnitTests/Controllers/TranslationControllerTests.cs +++ /dev/null @@ -1,60 +0,0 @@ -// TODO: These tests were for TranslationController which has been converted to Minimal API endpoints (TranslationEndpoints). -// The endpoint methods are now static extension methods and need different testing approaches. -// Consider testing the ITranslationService directly or using integration tests for the endpoints. - -/* -using FluentAssertions; -using Microsoft.AspNetCore.Mvc; -using Microsoft.Extensions.Logging; -using Moq; -using PoVicTranslate.Web.Models; -using PoVicTranslate.Web.Endpoints; // Note: Endpoints are now static extension classes -using PoVicTranslate.Web.Services; -using PoVicTranslate.Web.Services.Validation; -using Xunit; - -namespace Po.VicTranslate.UnitTests.Controllers; - -public class TranslationControllerTests -{ - private readonly Mock _mockTranslationService; - private readonly Mock _mockAudioSynthesisService; - private readonly Mock _mockTelemetryService; - private readonly Mock _mockInputValidator; - // TranslationController no longer exists - converted to TranslationEndpoints (Minimal API) - - public TranslationControllerTests() - { - _mockTranslationService = new Mock(); - _mockAudioSynthesisService = new Mock(); - _mockTelemetryService = new Mock(); - _mockInputValidator = new Mock(); - } - - [Fact] - public async Task Translate_WithValidRequest_ShouldReturnOkResult() - { - // TODO: Test via integration tests or test ITranslationService directly - } - - [Theory] - [InlineData(null)] - [InlineData("")] - public async Task Translate_WithEmptyText_ShouldReturnBadRequest(string? text) - { - // TODO: Test via integration tests or test ITranslationService directly - } - - [Fact] - public async Task Translate_ShouldCallTranslationService() - { - // TODO: Test via integration tests or test ITranslationService directly - } - - [Fact] - public async Task Translate_WhenServiceThrowsException_ShouldPropagate() - { - // TODO: Test via integration tests or test ITranslationService directly - } -} -*/ diff --git a/tests/Po.VicTranslate.UnitTests/Middleware/ApiResponseTimeMiddlewareTests.cs b/tests/Po.VicTranslate.UnitTests/Middleware/ApiResponseTimeMiddlewareTests.cs deleted file mode 100644 index ec8b405..0000000 --- a/tests/Po.VicTranslate.UnitTests/Middleware/ApiResponseTimeMiddlewareTests.cs +++ /dev/null @@ -1,221 +0,0 @@ -// TODO: ApiResponseTimeMiddleware no longer exists in the new project structure. -// The middleware functionality may have been replaced or moved. -// These tests need to be updated once the middleware location is identified. - -/* -using System.Diagnostics; -using Microsoft.AspNetCore.Http; -using Microsoft.Extensions.Logging; -using Moq; -// using PoVicTranslate.Web.Middleware; // Middleware namespace no longer exists -using PoVicTranslate.Web.Services; -using Xunit; - -namespace Po.VicTranslate.UnitTests.Middleware; - -public class ApiResponseTimeMiddlewareTests -{ - private readonly Mock> _mockLogger; - private readonly Mock _mockTelemetry; - private readonly Mock _mockNext; - - public ApiResponseTimeMiddlewareTests() - { - _mockLogger = new Mock>(); - _mockTelemetry = new Mock(); - _mockNext = new Mock(); - } - - [Fact] - public async Task InvokeAsync_TracksSuccessfulRequest() - { - // Arrange - var middleware = new ApiResponseTimeMiddleware(_mockNext.Object, _mockLogger.Object); - var context = new DefaultHttpContext(); - context.Request.Method = "GET"; - context.Request.Path = "/api/translation"; - context.Response.StatusCode = 200; - - _mockNext.Setup(next => next(context)) - .Returns(Task.CompletedTask); - - // Act - await middleware.InvokeAsync(context, _mockTelemetry.Object); - - // Assert - _mockTelemetry.Verify( - t => t.TrackApiResponse( - "GET /api/translation", - 200, - It.IsAny()), - Times.Once); - } - - [Fact] - public async Task InvokeAsync_TracksErrorResponse() - { - // Arrange - var middleware = new ApiResponseTimeMiddleware(_mockNext.Object, _mockLogger.Object); - var context = new DefaultHttpContext(); - context.Request.Method = "POST"; - context.Request.Path = "/api/lyrics"; - context.Response.StatusCode = 500; - - _mockNext.Setup(next => next(context)) - .Returns(Task.CompletedTask); - - // Act - await middleware.InvokeAsync(context, _mockTelemetry.Object); - - // Assert - _mockTelemetry.Verify( - t => t.TrackApiResponse( - "POST /api/lyrics", - 500, - It.IsAny()), - Times.Once); - } - - [Fact] - public async Task InvokeAsync_TracksDurationCorrectly() - { - // Arrange - var middleware = new ApiResponseTimeMiddleware(_mockNext.Object, _mockLogger.Object); - var context = new DefaultHttpContext(); - context.Request.Method = "GET"; - context.Request.Path = "/api/health"; - context.Response.StatusCode = 200; - - long capturedDuration = 0; - _mockTelemetry.Setup(t => t.TrackApiResponse(It.IsAny(), It.IsAny(), It.IsAny())) - .Callback((_, _, duration) => capturedDuration = duration); - - _mockNext.Setup(next => next(context)) - .Returns(async () => - { - await Task.Delay(100); // Simulate 100ms processing - }); - - // Act - await middleware.InvokeAsync(context, _mockTelemetry.Object); - - // Assert - Assert.True(capturedDuration >= 100, $"Duration should be at least 100ms, but was {capturedDuration}ms"); - Assert.True(capturedDuration < 200, $"Duration should be less than 200ms, but was {capturedDuration}ms"); - } - - [Fact] - public async Task InvokeAsync_LogsSlowRequests() - { - // Arrange - var middleware = new ApiResponseTimeMiddleware(_mockNext.Object, _mockLogger.Object); - var context = new DefaultHttpContext(); - context.Request.Method = "GET"; - context.Request.Path = "/api/slow-endpoint"; - context.Response.StatusCode = 200; - - _mockNext.Setup(next => next(context)) - .Returns(async () => - { - await Task.Delay(2100); // Simulate slow request (>2000ms) - }); - - // Act - await middleware.InvokeAsync(context, _mockTelemetry.Object); - - // Assert - _mockLogger.Verify( - logger => logger.Log( - LogLevel.Warning, - It.IsAny(), - It.Is((v, t) => v.ToString()!.Contains("Slow API request")), - It.IsAny(), - It.IsAny>()), - Times.Once); - } - - [Fact] - public async Task InvokeAsync_DoesNotLogFastRequests() - { - // Arrange - var middleware = new ApiResponseTimeMiddleware(_mockNext.Object, _mockLogger.Object); - var context = new DefaultHttpContext(); - context.Request.Method = "GET"; - context.Request.Path = "/api/fast-endpoint"; - context.Response.StatusCode = 200; - - _mockNext.Setup(next => next(context)) - .Returns(Task.CompletedTask); // Fast request (<2000ms) - - // Act - await middleware.InvokeAsync(context, _mockTelemetry.Object); - - // Assert - _mockLogger.Verify( - logger => logger.Log( - LogLevel.Warning, - It.IsAny(), - It.Is((v, t) => v.ToString()!.Contains("Slow API request")), - It.IsAny(), - It.IsAny>()), - Times.Never); - } - - [Fact] - public async Task InvokeAsync_TracksEvenWhenNextThrowsException() - { - // Arrange - var middleware = new ApiResponseTimeMiddleware(_mockNext.Object, _mockLogger.Object); - var context = new DefaultHttpContext(); - context.Request.Method = "GET"; - context.Request.Path = "/api/error"; - context.Response.StatusCode = 500; // Set by exception handler - - _mockNext.Setup(next => next(context)) - .ThrowsAsync(new InvalidOperationException("Test exception")); - - // Act & Assert - await Assert.ThrowsAsync( - async () => await middleware.InvokeAsync(context, _mockTelemetry.Object)); - - // Telemetry should still be tracked in finally block - _mockTelemetry.Verify( - t => t.TrackApiResponse( - "GET /api/error", - It.IsAny(), - It.IsAny()), - Times.Once); - } - - [Theory] - [InlineData("GET", "/api/lyrics")] - [InlineData("POST", "/api/translation")] - [InlineData("DELETE", "/api/lyrics/management/songs/123")] - [InlineData("PUT", "/api/speech")] - public async Task InvokeAsync_TracksVariousEndpoints(string method, string path) - { - // Arrange - var middleware = new ApiResponseTimeMiddleware(_mockNext.Object, _mockLogger.Object); - var context = new DefaultHttpContext(); - context.Request.Method = method; - context.Request.Path = path; - context.Response.StatusCode = 200; - - _mockNext.Setup(next => next(context)) - .Returns(Task.CompletedTask); - - var expectedEndpoint = $"{method} {path}"; - - // Act - await middleware.InvokeAsync(context, _mockTelemetry.Object); - - // Assert - _mockTelemetry.Verify( - t => t.TrackApiResponse( - expectedEndpoint, - 200, - It.IsAny()), - Times.Once); - } -} -*/ diff --git a/tests/Po.VicTranslate.UnitTests/Services/Lyrics/LyricsServiceTests.cs b/tests/Po.VicTranslate.UnitTests/Services/Lyrics/LyricsServiceTests.cs new file mode 100644 index 0000000..ebee08f --- /dev/null +++ b/tests/Po.VicTranslate.UnitTests/Services/Lyrics/LyricsServiceTests.cs @@ -0,0 +1,148 @@ +using FluentAssertions; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.Logging; +using Moq; +using PoVicTranslate.Web.Services; +using System.Text.Json; +using Xunit; +using PoVicTranslate.Web.Models; + +namespace Po.VicTranslate.UnitTests.Services.Lyrics; + +public class LyricsServiceTests : IDisposable +{ + private readonly Mock _mockEnvironment; + private readonly Mock> _mockLogger; + private readonly string _tempDirectory; + private readonly string _lyricsFilePath; + + public LyricsServiceTests() + { + _mockEnvironment = new Mock(); + _mockLogger = new Mock>(); + + // Setup temp directory + _tempDirectory = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); + Directory.CreateDirectory(_tempDirectory); + + // Mock ContentRootPath + _mockEnvironment.Setup(e => e.ContentRootPath).Returns(_tempDirectory); + + // Ensure Data directory exists + var dataDir = Path.Combine(_tempDirectory, "Data"); + Directory.CreateDirectory(dataDir); + _lyricsFilePath = Path.Combine(dataDir, "lyrics-collection.json"); + } + + public void Dispose() + { + if (Directory.Exists(_tempDirectory)) + { + try { Directory.Delete(_tempDirectory, true); } catch { } + } + GC.SuppressFinalize(this); + } + + private void CreateLyricsFile(LyricsCollection collection) + { + var json = JsonSerializer.Serialize(collection); + File.WriteAllText(_lyricsFilePath, json); + } + + [Fact] + public async Task GetAvailableSongsAsync_ShouldReturnSortedTitles() + { + // Arrange + var collection = new LyricsCollection + { + Songs = new List + { + new SongEntry { Title = "Song B", Lyrics = "Lyrics B" }, + new SongEntry { Title = "Song A", Lyrics = "Lyrics A" } + } + }; + CreateLyricsFile(collection); + + var service = new LyricsService(_mockEnvironment.Object, _mockLogger.Object); + + // Act + var songs = await service.GetAvailableSongsAsync(); + + // Assert + songs.Should().HaveCount(2); + songs[0].Should().Be("Song A"); + songs[1].Should().Be("Song B"); + } + + [Fact] + public async Task GetLyricsAsync_WithValidId_ShouldReturnLyrics() + { + // Arrange + var collection = new LyricsCollection + { + Songs = new List + { + new SongEntry { Title = "TestSong", Lyrics = "These are the lyrics." } + } + }; + CreateLyricsFile(collection); + + var service = new LyricsService(_mockEnvironment.Object, _mockLogger.Object); + + // Act + var lyrics = await service.GetLyricsAsync("TestSong.txt"); + + // Assert + lyrics.Should().Be("These are the lyrics."); + } + + [Fact] + public async Task GetLyricsAsync_WithInvalidId_ShouldReturnNull() + { + // Arrange + var collection = new LyricsCollection { Songs = new List() }; + CreateLyricsFile(collection); + + var service = new LyricsService(_mockEnvironment.Object, _mockLogger.Object); + + // Act + var lyrics = await service.GetLyricsAsync("NonExistent"); + + // Assert + lyrics.Should().BeNull(); + } + + [Fact] + public async Task GetLyricsAsync_ShouldHandleMaxWordsLimitation() + { + // Arrange + var longLyrics = string.Join(" ", Enumerable.Range(0, 300).Select(i => $"word{i}")); + var collection = new LyricsCollection + { + Songs = new List + { + new SongEntry { Title = "LongSong", Lyrics = longLyrics } + } + }; + CreateLyricsFile(collection); + + var service = new LyricsService(_mockEnvironment.Object, _mockLogger.Object); + + // Act + var lyrics = await service.GetLyricsAsync("LongSong"); + + // Assert + lyrics.Should().EndWith("..."); + // 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. + lyrics.Length.Should().BeLessThan(longLyrics.Length); + } +} diff --git a/tests/Po.VicTranslate.UnitTests/Services/TranslationServiceTests.cs b/tests/Po.VicTranslate.UnitTests/Services/TranslationServiceTests.cs index 5c80cae..ebfc48f 100644 --- a/tests/Po.VicTranslate.UnitTests/Services/TranslationServiceTests.cs +++ b/tests/Po.VicTranslate.UnitTests/Services/TranslationServiceTests.cs @@ -4,8 +4,13 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Moq; +using Azure.AI.OpenAI; +using OpenAI; +using OpenAI.Chat; using PoVicTranslate.Web.Configuration; using PoVicTranslate.Web.Services; +using System.ClientModel; +using System.ClientModel.Primitives; using Xunit; namespace Po.VicTranslate.UnitTests.Services; @@ -105,4 +110,87 @@ public void Constructor_ShouldLogSuccessfulInitialization() It.IsAny>()), Times.Once); } + + [Fact] + public async Task TranslateToVictorianEnglishAsync_WithValidText_ShouldReturnTranslation() + { + // Arrange + var mockChatClient = new Mock("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(); + mockResponse.Setup(r => r.Status).Returns(200); + + var clientResult = ClientResult.FromValue(chatCompletion, mockResponse.Object); + + mockChatClient.Setup(x => x.CompleteChatAsync( + It.IsAny())) + .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("gpt-4o", new ApiKeyCredential("key"), new OpenAIClientOptions()); + + var mockResponse = new Mock(); + mockResponse.Setup(r => r.Status).Returns(429); + + var exception = new ClientResultException(mockResponse.Object); + + mockChatClient.Setup(x => x.CompleteChatAsync( + It.IsAny())) + .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("gpt-4o", new ApiKeyCredential("key"), new OpenAIClientOptions()); + + mockChatClient.Setup(x => x.CompleteChatAsync( + It.IsAny())) + .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"); + } }