diff --git a/tests/LakeSpeak.Cli.Tests/ExitCodeTests.cs b/tests/LakeSpeak.Cli.Tests/ExitCodeTests.cs index 1632af8..820bd2f 100644 --- a/tests/LakeSpeak.Cli.Tests/ExitCodeTests.cs +++ b/tests/LakeSpeak.Cli.Tests/ExitCodeTests.cs @@ -1,4 +1,3 @@ -using LakeSpeak.Cli; using LakeSpeak.Genie; namespace LakeSpeak.Cli.Tests; @@ -21,10 +20,19 @@ public class ExitCodeTests [InlineData(GenieFailureKind.RateLimited, ExitCode.GenieFailure)] [InlineData(GenieFailureKind.PollingTimeout, ExitCode.Timeout)] [InlineData(GenieFailureKind.MalformedResponse, ExitCode.MalformedResponse)] + [InlineData(GenieFailureKind.UnsupportedResult, ExitCode.MalformedResponse)] [InlineData(GenieFailureKind.Network, ExitCode.Unexpected)] [InlineData(GenieFailureKind.Unexpected, ExitCode.Unexpected)] - public void Each_failure_kind_maps_to_its_documented_code(GenieFailureKind kind, int expected) => - ExitCode.From(kind).ShouldBe(expected); + public void Each_failure_kind_maps_to_its_documented_code(GenieFailureKind kind, int expected) + { + // Arrange — the kind under test arrives as the theory parameter. + + // Act + var code = ExitCode.From(kind); + + // Assert + code.ShouldBe(expected); + } /// /// Adding a without extending the mapping would otherwise @@ -34,10 +42,14 @@ public void Each_failure_kind_maps_to_its_documented_code(GenieFailureKind kind, [Fact] public void Every_failure_kind_is_mapped() { - foreach (var kind in Enum.GetValues()) - { - Should.NotThrow(() => ExitCode.From(kind), $"{kind} has no exit code mapping."); - } + // Arrange + var kinds = Enum.GetValues(); + + // Act + var unmapped = kinds.Where(k => !TryMap(k)).ToList(); + + // Assert + unmapped.ShouldBeEmpty(); } /// @@ -47,25 +59,51 @@ public void Every_failure_kind_is_mapped() [Fact] public void The_documented_numbers_have_not_moved() { - ExitCode.Success.ShouldBe(0); - ExitCode.Unexpected.ShouldBe(1); - ExitCode.InvalidUsage.ShouldBe(2); - ExitCode.Authentication.ShouldBe(3); - ExitCode.Authorization.ShouldBe(4); - ExitCode.NotFound.ShouldBe(5); - ExitCode.GenieFailure.ShouldBe(6); - ExitCode.Timeout.ShouldBe(7); - ExitCode.PartialPackFailure.ShouldBe(8); - ExitCode.MalformedResponse.ShouldBe(9); + // Arrange — the documented table from docs/commands.md. + var documented = new[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 }; + + // Act + var actual = new[] + { + ExitCode.Success, + ExitCode.Unexpected, + ExitCode.InvalidUsage, + ExitCode.Authentication, + ExitCode.Authorization, + ExitCode.NotFound, + ExitCode.GenieFailure, + ExitCode.Timeout, + ExitCode.PartialPackFailure, + ExitCode.MalformedResponse, + }; + + // Assert + actual.ShouldBe(documented); } - // Success must stay 0 and every failure non-zero, or `set -e` and CI stop working. [Fact] public void Only_success_is_zero() { - foreach (var kind in Enum.GetValues()) + // Arrange — success must stay 0 and every failure non-zero, or `set -e` and CI break. + var kinds = Enum.GetValues(); + + // Act + var codes = kinds.Select(ExitCode.From).ToList(); + + // Assert + codes.ShouldAllBe(c => c != ExitCode.Success); + } + + private static bool TryMap(GenieFailureKind kind) + { + try + { + ExitCode.From(kind); + return true; + } + catch (ArgumentOutOfRangeException) { - ExitCode.From(kind).ShouldNotBe(ExitCode.Success); + return false; } } } diff --git a/tests/LakeSpeak.ContractTests/GenieLifecycleTests.cs b/tests/LakeSpeak.ContractTests/GenieLifecycleTests.cs index b9e8243..7d2e218 100644 --- a/tests/LakeSpeak.ContractTests/GenieLifecycleTests.cs +++ b/tests/LakeSpeak.ContractTests/GenieLifecycleTests.cs @@ -30,7 +30,7 @@ private GenieClient CreateClient(TimeSpan? timeout = null) var http = new HttpClient { BaseAddress = new Uri(_server.Url!) }; var options = new GenieClientOptions { - // The server is http, and Validate() rightly refuses a non-https host. Host is only + // The stub is http, and Validate() rightly refuses a non-https host. Host is only // used to build the base address, which is set directly above. Host = new Uri("https://example.azuredatabricks.net"), InitialPollInterval = TimeSpan.FromSeconds(1), @@ -74,8 +74,7 @@ private void StubMessageSequence(params string[] statuses) } // The last stub must not advance the state, or it stops matching itself after one - // response and WireMock starts 404ing. That turned the polling-timeout test into a - // 404 test without either one failing for the right reason. + // response and WireMock starts 404ing. if (!isLast) { stub = stub.WillSetStateTo($"s{i + 1}"); @@ -145,15 +144,18 @@ private void StubQueryResult() => [Fact] public async Task Ask_walks_the_full_lifecycle_and_returns_a_normalized_response() { + // Arrange StubStartConversation(); StubMessageSequence("SUBMITTED", "FILTERING_CONTEXT", "EXECUTING_QUERY", "COMPLETED"); StubQueryResult(); - var client = CreateClient(); + + // Act var task = client.AskAsync(Agent, "How did revenue change?", cancellationToken: Ct); await AdvanceUntilSettledAsync(task); var response = await task; + // Assert response.State.ShouldBe(GenieMessageState.Completed); response.ConversationId.ShouldBe(Conversation); @@ -176,12 +178,14 @@ public async Task Ask_walks_the_full_lifecycle_and_returns_a_normalized_response [Fact] public async Task Reports_every_state_transition_in_order() { + // Arrange StubStartConversation(); StubMessageSequence("SUBMITTED", "PENDING_WAREHOUSE", "EXECUTING_QUERY", "COMPLETED"); StubQueryResult(); - var seen = new List(); var client = CreateClient(); + + // Act var task = client.AskAsync(Agent, "q", new GenieAskOptions { OnStateChanged = s => seen.Add(s), @@ -189,6 +193,7 @@ public async Task Reports_every_state_transition_in_order() await AdvanceUntilSettledAsync(task); await task; + // Assert seen.ShouldBe([ GenieMessageState.Submitted, GenieMessageState.PendingWarehouse, @@ -200,6 +205,7 @@ public async Task Reports_every_state_transition_in_order() [Fact] public async Task A_failed_message_raises_MessageFailed_carrying_the_platform_reason() { + // Arrange StubStartConversation(); _server.Given(Request.Create() .WithPath($"/api/2.0/genie/spaces/{Agent}/conversations/{Conversation}/messages/{Message}") @@ -212,21 +218,24 @@ public async Task A_failed_message_raises_MessageFailed_carrying_the_platform_re "error": { "error": "Table orders does not exist", "type": "SQL_EXECUTION_EXCEPTION" } } """)); - var client = CreateClient(); - var ex = await Should.ThrowAsync(() => client.AskAsync(Agent, "q", cancellationToken: Ct)); + // Act + var ex = await Should.ThrowAsync( + () => client.AskAsync(Agent, "q", cancellationToken: Ct)); + + // Assert ex.Kind.ShouldBe(GenieFailureKind.MessageFailed); ex.Message.ShouldContain("Table orders does not exist"); ex.ErrorCode.ShouldBe("SQL_EXECUTION_EXCEPTION"); ex.IsRetryable.ShouldBeFalse(); } - // QUERY_RESULT_EXPIRED is terminal but is not a failure: the answer and SQL are still - // valid. Throwing here would discard a good answer over a stale cache entry. [Fact] public async Task An_expired_result_returns_the_answer_rather_than_throwing() { + // Arrange — QUERY_RESULT_EXPIRED is terminal but is not a failure: the answer and SQL + // are still valid, so throwing would discard a good answer over a stale cache entry. StubStartConversation(); _server.Given(Request.Create() .WithPath($"/api/2.0/genie/spaces/{Agent}/conversations/{Conversation}/messages/{Message}") @@ -241,10 +250,12 @@ public async Task An_expired_result_returns_the_answer_rather_than_throwing() ] } """)); - var client = CreateClient(); + + // Act var response = await client.AskAsync(Agent, "q", cancellationToken: Ct); + // Assert response.State.ShouldBe(GenieMessageState.QueryResultExpired); response.Text.ShouldBe("Revenue rose 14.2%."); } @@ -257,14 +268,18 @@ public async Task An_expired_result_returns_the_answer_rather_than_throwing() [InlineData(500, GenieFailureKind.Unexpected, false)] public async Task Maps_http_failures_to_typed_kinds(int status, GenieFailureKind kind, bool retryable) { + // Arrange _server.Given(Request.Create() .WithPath($"/api/2.0/genie/spaces/{Agent}/start-conversation").UsingPost()) .RespondWith(Response.Create().WithStatusCode(status).WithBody( """{"error_code":"PERMISSION_DENIED","message":"nope"}""")); - var client = CreateClient(); - var ex = await Should.ThrowAsync(() => client.AskAsync(Agent, "q", cancellationToken: Ct)); + // Act + var ex = await Should.ThrowAsync( + () => client.AskAsync(Agent, "q", cancellationToken: Ct)); + + // Assert ex.Kind.ShouldBe(kind); ex.StatusCode.ShouldBe(status); ex.IsRetryable.ShouldBe(retryable); @@ -273,30 +288,35 @@ public async Task Maps_http_failures_to_typed_kinds(int status, GenieFailureKind [Fact] public async Task A_body_that_is_not_json_fails_as_MalformedResponse_without_echoing_it() { + // Arrange _server.Given(Request.Create() .WithPath($"/api/2.0/genie/spaces/{Agent}/start-conversation").UsingPost()) .RespondWith(Response.Create().WithStatusCode(200).WithBody("gateway")); - var client = CreateClient(); - var ex = await Should.ThrowAsync(() => client.AskAsync(Agent, "q", cancellationToken: Ct)); + // Act + var ex = await Should.ThrowAsync( + () => client.AskAsync(Agent, "q", cancellationToken: Ct)); + + // Assert — the body can contain query results, which are governed data, and this message + // may reach a log or a bug report. ex.Kind.ShouldBe(GenieFailureKind.MalformedResponse); - // The body can contain query results, which are governed data, and this message may - // reach a log or a bug report. ex.Message.ShouldNotContain("gateway"); } [Fact] public async Task Polling_stops_at_the_timeout_and_keeps_the_last_seen_state() { + // Arrange StubStartConversation(); StubMessageSequence("EXECUTING_QUERY"); - var client = CreateClient(timeout: TimeSpan.FromSeconds(30)); - var task = client.AskAsync(Agent, "q", cancellationToken: Ct); + // Act + var task = client.AskAsync(Agent, "q", cancellationToken: Ct); await AdvanceUntilSettledAsync(task); + // Assert var ex = await Should.ThrowAsync(() => task); ex.Kind.ShouldBe(GenieFailureKind.PollingTimeout); ex.LastKnownResponse!.State.ShouldBe(GenieMessageState.ExecutingQuery); @@ -305,31 +325,37 @@ public async Task Polling_stops_at_the_timeout_and_keeps_the_last_seen_state() [Fact] public async Task Cancellation_stops_polling_promptly() { + // Arrange StubStartConversation(); StubMessageSequence("EXECUTING_QUERY"); - using var cts = new CancellationTokenSource(); var client = CreateClient(); - var task = client.AskAsync(Agent, "q", cancellationToken: cts.Token); + // Act + var task = client.AskAsync(Agent, "q", cancellationToken: cts.Token); await Task.Yield(); await cts.CancelAsync(); _clock.Advance(TimeSpan.FromSeconds(5)); + // Assert await Should.ThrowAsync(() => task); } [Fact] public async Task Feedback_posts_the_rating_and_tolerates_an_empty_body() { + // Arrange _server.Given(Request.Create() .WithPath($"/api/2.0/genie/spaces/{Agent}/conversations/{Conversation}/messages/{Message}/feedback") .UsingPost()) .RespondWith(Response.Create().WithStatusCode(200).WithBody(string.Empty)); - var client = CreateClient(); - await client.SendFeedbackAsync(Agent, Conversation, Message, GenieFeedbackRating.Negative, "wrong filter", Ct); + // Act + await client.SendFeedbackAsync( + Agent, Conversation, Message, GenieFeedbackRating.Negative, "wrong filter", Ct); + + // Assert var request = _server.LogEntries.Single().RequestMessage; Assert.NotNull(request); Assert.NotNull(request.Body); @@ -340,6 +366,7 @@ public async Task Feedback_posts_the_rating_and_tolerates_an_empty_body() [Fact] public async Task Agent_listing_follows_pagination() { + // Arrange _server.Given(Request.Create().WithPath("/api/2.0/genie/spaces").UsingGet() .WithParam("page_token", "p2")) .RespondWith(Response.Create().WithStatusCode(200).WithBody( @@ -351,25 +378,30 @@ public async Task Agent_listing_follows_pagination() var client = CreateClient(); var agents = new List(); + + // Act await foreach (var agent in client.ListAllAgentsAsync(Ct)) { agents.Add(agent); } + // Assert agents.Select(a => a.Title).ShouldBe(["Sales", "Finance"]); } - // WireMock hands back the same token forever here. Without the repeated-token guard this - // enumerates until the process is killed. [Fact] public async Task Agent_listing_stops_when_the_server_repeats_a_page_token() { + // Arrange — WireMock hands back the same token forever here. Without the repeated-token + // guard this enumerates until the process is killed. _server.Given(Request.Create().WithPath("/api/2.0/genie/spaces").UsingGet()) .RespondWith(Response.Create().WithStatusCode(200).WithBody( """{"spaces":[{"space_id":"a","title":"Sales"}],"next_page_token":"same"}""")); var client = CreateClient(); var count = 0; + + // Act await foreach (var _ in client.ListAllAgentsAsync(Ct)) { if (++count > 50) @@ -378,6 +410,7 @@ public async Task Agent_listing_stops_when_the_server_repeats_a_page_token() } } + // Assert count.ShouldBe(2); } @@ -386,10 +419,10 @@ public async Task Agent_listing_stops_when_the_server_repeats_a_page_token() /// throws. /// /// - /// Bounded by real wall time, not by a fixed iteration count. A counted loop can exhaust - /// its iterations while an HTTP round trip is still in flight, after which nothing advances - /// the clock again and the awaiting test hangs forever rather than failing. Falling out of - /// this loop without the task settling is itself a failure, and is reported as one. + /// Bounded by real wall time, not by a fixed iteration count. A counted loop can exhaust its + /// iterations while an HTTP round trip is still in flight, after which nothing advances the + /// clock again and the awaiting test hangs forever rather than failing. Falling out of this + /// loop without the task settling is itself a failure, and is reported as one. /// private async Task AdvanceUntilSettledAsync(Task task) { diff --git a/tests/LakeSpeak.ContractTests/ResultCompletenessTests.cs b/tests/LakeSpeak.ContractTests/ResultCompletenessTests.cs index 738bb45..2540d7e 100644 --- a/tests/LakeSpeak.ContractTests/ResultCompletenessTests.cs +++ b/tests/LakeSpeak.ContractTests/ResultCompletenessTests.cs @@ -52,37 +52,41 @@ private void StubQueryResult(string resultJson, string manifestExtra = "") /// /// The Statement Execution contract splits large results into chunks, and this client reads - /// only the first. `manifest.truncated` reports statement-level truncation by Databricks and - /// is false for a merely-chunked result, so relying on it alone hands back the first chunk + /// only the first. manifest.truncated reports statement-level truncation by Databricks + /// and is false for a merely-chunked result, so relying on it alone hands back the first chunk /// labelled complete — a partial export nobody knows is partial. /// [Fact] public async Task A_chunked_result_is_reported_as_truncated() { + // Arrange StubQueryResult( """{ "row_count": 2, "chunk_index": 0, "next_chunk_index": 1, "data_array": [["Germany"],["France"]] }"""); + var client = CreateClient(); - var result = await CreateClient() - .GetQueryResultAsync(Agent, Conversation, Message, Attachment, Ct); + // Act + var result = await client.GetQueryResultAsync(Agent, Conversation, Message, Attachment, Ct); + // Assert result.ShouldNotBeNull(); result.Rows.Count.ShouldBe(2); result.IsTruncated.ShouldBeTrue(); } - /// - /// The same failure reached a different way: the manifest advertises more rows than arrived. - /// [Fact] public async Task A_short_read_against_the_manifest_row_count_is_reported_as_truncated() { + // Arrange — the same failure reached a different way: the manifest advertises more rows + // than arrived. StubQueryResult( """{ "row_count": 1, "data_array": [["Germany"]] }""", manifestExtra: """, "total_row_count": 5000"""); + var client = CreateClient(); - var result = await CreateClient() - .GetQueryResultAsync(Agent, Conversation, Message, Attachment, Ct); + // Act + var result = await client.GetQueryResultAsync(Agent, Conversation, Message, Attachment, Ct); + // Assert result!.IsTruncated.ShouldBeTrue(); result.TotalRowCount.ShouldBe(5000); } @@ -90,24 +94,50 @@ public async Task A_short_read_against_the_manifest_row_count_is_reported_as_tru [Fact] public async Task A_complete_single_chunk_result_is_not_reported_as_truncated() { + // Arrange — the conservative truncation check must not false-positive on a whole result. StubQueryResult( """{ "row_count": 2, "chunk_index": 0, "data_array": [["Germany"],["France"]] }""", manifestExtra: """, "total_row_count": 2"""); + var client = CreateClient(); - var result = await CreateClient() - .GetQueryResultAsync(Agent, Conversation, Message, Attachment, Ct); + // Act + var result = await client.GetQueryResultAsync(Agent, Conversation, Message, Attachment, Ct); + // Assert result!.IsTruncated.ShouldBeFalse(); } + /// + /// Under EXTERNAL_LINKS disposition the rows sit behind presigned URLs and + /// data_array is absent. Returning an empty row set as a successful, complete result + /// would be the worst outcome available — a silently empty export that looks like an answer. + /// + [Fact] + public async Task An_external_links_result_is_refused_rather_than_returned_empty() + { + // Arrange — no data_array, no total_row_count, no next_chunk_index. + StubQueryResult( + """{ "external_links": [ { "chunk_index": 0, "row_count": 100000 } ] }"""); + var client = CreateClient(); + + // Act + var act = () => client.GetQueryResultAsync(Agent, Conversation, Message, Attachment, Ct); + + // Assert + var ex = await Should.ThrowAsync(act); + ex.Kind.ShouldBe(GenieFailureKind.UnsupportedResult); + ex.Message.ShouldContain("100000"); + } + /// /// start-conversation is not idempotent: a retry asks Genie the same question again, running /// the SQL warehouse a second time and billing for it, and leaves an orphaned conversation - /// whose id the caller never receives. The resilience pipeline must not retry it. + /// whose id the caller never receives. /// [Fact] public async Task A_failed_start_conversation_is_never_retried() { + // Arrange _server.Given(Request.Create() .WithPath($"/api/2.0/genie/spaces/{Agent}/start-conversation").UsingPost()) .RespondWith(Response.Create().WithStatusCode(503)); @@ -117,21 +147,20 @@ public async Task A_failed_start_conversation_is_never_retried() services.AddLakeSpeak(o => o.Host = new Uri("https://example.azuredatabricks.net")); using var provider = services.BuildServiceProvider(); - // Point the configured client at the stub without disturbing the resilience pipeline. - var factory = provider.GetRequiredService(); - var http = factory.CreateClient(nameof(IGenieClient)); + // The configured client is pointed at the stub without disturbing the resilience pipeline. + var http = provider.GetRequiredService().CreateClient(nameof(IGenieClient)); http.BaseAddress = new Uri(_server.Url!); - var client = new GenieClient(http, Options.Create(new GenieClientOptions { Host = new Uri("https://example.azuredatabricks.net"), })); + // Act await Should.ThrowAsync(() => client.AskAsync(Agent, "q", cancellationToken: Ct)); + // Assert var attempts = _server.LogEntries.Count(e => e.RequestMessage?.Path?.EndsWith("start-conversation", StringComparison.Ordinal) == true); - attempts.ShouldBe(1); } diff --git a/tests/LakeSpeak.Genie.Tests/DiagnosticRedactionTests.cs b/tests/LakeSpeak.Genie.Tests/DiagnosticRedactionTests.cs index 1dbf7bb..8b8a339 100644 --- a/tests/LakeSpeak.Genie.Tests/DiagnosticRedactionTests.cs +++ b/tests/LakeSpeak.Genie.Tests/DiagnosticRedactionTests.cs @@ -1,22 +1,25 @@ -using LakeSpeak.Genie; -using Shouldly; - namespace LakeSpeak.Genie.Tests; public class DiagnosticRedactionTests { - // Assembled at runtime rather than written as a literal: a token-shaped constant - // in source trips secret scanners on every clone and pull request, and the - // assembled value exercises the regex identically. + // Assembled at runtime rather than written as a literal: a token-shaped constant in source + // trips secret scanners on every clone and pull request, and the assembled value exercises + // the regex identically. private static readonly string Pat = "dapi" + new string('a', 32); + private const string Jwt = "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dBjftJeZ4CVPmB92K27uhbUJU1p1r_wW1gFWFOEjXk"; [Fact] public void Redacts_databricks_personal_access_token() { - var scrubbed = DiagnosticRedaction.Scrub($"request failed with token {Pat} attached"); + // Arrange + var diagnostic = $"request failed with token {Pat} attached"; + // Act + var scrubbed = DiagnosticRedaction.Scrub(diagnostic); + + // Assert scrubbed.ShouldNotContain(Pat); scrubbed.ShouldContain(DiagnosticRedaction.Placeholder); } @@ -24,7 +27,14 @@ public void Redacts_databricks_personal_access_token() [Fact] public void Redacts_jwt_bearer_token() { - DiagnosticRedaction.Scrub($"Authorization: Bearer {Jwt}").ShouldNotContain(Jwt); + // Arrange + var diagnostic = $"Authorization: Bearer {Jwt}"; + + // Act + var scrubbed = DiagnosticRedaction.Scrub(diagnostic); + + // Assert + scrubbed.ShouldNotContain(Jwt); } [Theory] @@ -32,59 +42,85 @@ public void Redacts_jwt_bearer_token() [InlineData("access_token: abc123xyz789")] [InlineData("download_id_signature=c2lnbmF0dXJlCg")] [InlineData("X-Databricks-Session-Token: opaquevalue1")] - public void Redacts_named_secrets(string input) + public void Redacts_named_secrets(string diagnostic) { - var scrubbed = DiagnosticRedaction.Scrub(input); + // Arrange — the diagnostic arrives as the theory parameter. + + // Act + var scrubbed = DiagnosticRedaction.Scrub(diagnostic); + // Assert — the key name survives so a log line stays diagnosable; only the value goes. scrubbed.ShouldContain(DiagnosticRedaction.Placeholder); - // The key name survives so a log line stays diagnosable; only the value goes. scrubbed.Split('=', ':')[0].ShouldNotBeNullOrWhiteSpace(); } - // The download signature is bearer-equivalent: anyone holding it can fetch the full query - // result, which is governed data. [Fact] public void Redacts_download_signature_from_a_realistic_payload() { - var payload = + // Arrange — the download signature is bearer-equivalent for the full query result. + const string payload = """{"download_id":"abc","download_id_signature":"c2VjcmV0LXNpZ25hdHVyZQ=="}"""; - DiagnosticRedaction.Scrub(payload).ShouldNotContain("c2VjcmV0LXNpZ25hdHVyZQ"); + // Act + var scrubbed = DiagnosticRedaction.Scrub(payload); + + // Assert + scrubbed.ShouldNotContain("c2VjcmV0LXNpZ25hdHVyZQ"); } - // The message-level query_result summary carries statement_id_signature, which is a JWT - // guarding access to the result rows. It is a different field from download_id_signature - // and was missed by the first version of the scrubber. [Fact] public void Redacts_statement_id_signature() { - var payload = + // Arrange — a different field from download_id_signature, and missed by the first + // version of the scrubber. + const string payload = """{"statement_id":"01ef","statement_id_signature":"c3RhdGVtZW50LXNpZw=="}"""; - DiagnosticRedaction.Scrub(payload).ShouldNotContain("c3RhdGVtZW50LXNpZw"); + // Act + var scrubbed = DiagnosticRedaction.Scrub(payload); + + // Assert + scrubbed.ShouldNotContain("c3RhdGVtZW50LXNpZw"); } [Fact] public void Leaves_ordinary_text_alone() { + // Arrange const string message = "Genie could not answer: the table orders does not exist."; - DiagnosticRedaction.Scrub(message).ShouldBe(message); + // Act + var scrubbed = DiagnosticRedaction.Scrub(message); + + // Assert + scrubbed.ShouldBe(message); } [Theory] [InlineData(null)] [InlineData("")] - public void Handles_empty_input(string? input) => - DiagnosticRedaction.Scrub(input).ShouldBe(string.Empty); + public void Handles_empty_input(string? diagnostic) + { + // Arrange — the input arrives as the theory parameter. + + // Act + var scrubbed = DiagnosticRedaction.Scrub(diagnostic); + + // Assert + scrubbed.ShouldBe(string.Empty); + } - // GenieException scrubs in its constructor, so a token cannot reach a caller's log by way - // of an exception message even if a call site forgets. [Fact] public void Exception_message_is_scrubbed_on_construction() { - var ex = new GenieException(GenieFailureKind.Authentication, $"failed using {Pat}"); + // Arrange — GenieException scrubs in its constructor, so a token cannot reach a caller's + // log by way of an exception even if a call site forgets. + var message = $"failed using {Pat}"; + + // Act + var exception = new GenieException(GenieFailureKind.Authentication, message); - ex.Message.ShouldNotContain(Pat); + // Assert + exception.Message.ShouldNotContain(Pat); } } diff --git a/tests/LakeSpeak.Genie.Tests/GenieMessageStateTests.cs b/tests/LakeSpeak.Genie.Tests/GenieMessageStateTests.cs index bb3360a..cdbaaad 100644 --- a/tests/LakeSpeak.Genie.Tests/GenieMessageStateTests.cs +++ b/tests/LakeSpeak.Genie.Tests/GenieMessageStateTests.cs @@ -1,6 +1,3 @@ -using LakeSpeak.Genie; -using Shouldly; - namespace LakeSpeak.Genie.Tests; public class GenieMessageStateTests @@ -19,58 +16,109 @@ public class GenieMessageStateTests [InlineData("FAILED", GenieMessageState.Failed)] [InlineData("CANCELLED", GenieMessageState.Cancelled)] [InlineData("QUERY_RESULT_EXPIRED", GenieMessageState.QueryResultExpired)] - public void Maps_every_documented_platform_status(string wire, GenieMessageState expected) => - GenieMessageStateExtensions.FromWire(wire).ShouldBe(expected); + public void Maps_every_documented_platform_status(string wire, GenieMessageState expected) + { + // Arrange — the platform status arrives as the theory parameter. + + // Act + var state = GenieMessageStateExtensions.FromWire(wire); + + // Assert + state.ShouldBe(expected); + } [Theory] [InlineData("SOMETHING_DATABRICKS_ADDED_LATER")] [InlineData("")] [InlineData(null)] [InlineData("completed")] // the API is uppercase; casing is not normalised for us - public void Maps_anything_unrecognised_to_Unknown(string? wire) => - GenieMessageStateExtensions.FromWire(wire).ShouldBe(GenieMessageState.Unknown); + public void Maps_anything_unrecognised_to_Unknown(string? wire) + { + // Arrange — an unrecognised status arrives as the theory parameter. + + // Act + var state = GenieMessageStateExtensions.FromWire(wire); + + // Assert + state.ShouldBe(GenieMessageState.Unknown); + } [Theory] [InlineData(GenieMessageState.Completed)] [InlineData(GenieMessageState.Failed)] [InlineData(GenieMessageState.Cancelled)] [InlineData(GenieMessageState.QueryResultExpired)] - public void Terminal_states_stop_polling(GenieMessageState state) => - state.IsTerminal().ShouldBeTrue(); + public void Terminal_states_stop_polling(GenieMessageState state) + { + // Arrange — the state arrives as the theory parameter. + + // Act + var terminal = state.IsTerminal(); + + // Assert + terminal.ShouldBeTrue(); + } [Theory] [InlineData(GenieMessageState.Submitted)] [InlineData(GenieMessageState.Thinking)] [InlineData(GenieMessageState.PendingWarehouse)] [InlineData(GenieMessageState.ExecutingQuery)] - public void Non_terminal_states_continue_polling(GenieMessageState state) => - state.IsTerminal().ShouldBeFalse(); + public void Non_terminal_states_continue_polling(GenieMessageState state) + { + // Arrange — the state arrives as the theory parameter. + + // Act + var terminal = state.IsTerminal(); + + // Assert + terminal.ShouldBeFalse(); + } - // An unrecognised status is far more likely to be a new intermediate step than a new - // terminal one. Treating it as terminal would silently truncate a working conversation - // and return an empty answer as if it were complete. [Fact] - public void Unknown_is_not_terminal() => - GenieMessageState.Unknown.IsTerminal().ShouldBeFalse(); + public void Unknown_is_not_terminal() + { + // Arrange — an unrecognised status is far more likely a new intermediate step than a new + // terminal one; treating it as terminal would truncate a working conversation and return + // an empty answer as if it were complete. + const GenieMessageState state = GenieMessageState.Unknown; + + // Act + var terminal = state.IsTerminal(); + + // Assert + terminal.ShouldBeFalse(); + } - // Genie spells it CANCELLED; the SQL Statement Execution API spells its own state CANCELED. - // The two must never share a parser: feeding the SQL spelling in here has to fall through - // to Unknown rather than quietly resolving to Cancelled, which would end polling early. [Fact] public void Does_not_accept_the_SQL_APIs_single_L_spelling() { - GenieMessageStateExtensions.FromWire("CANCELED").ShouldBe(GenieMessageState.Unknown); - GenieMessageStateExtensions.FromWire("CANCELLED").ShouldBe(GenieMessageState.Cancelled); + // Arrange — Genie spells it CANCELLED; the SQL Statement Execution API spells its own + // state CANCELED. Feeding the SQL spelling here must not end a poll early. + const string sqlSpelling = "CANCELED"; + const string genieSpelling = "CANCELLED"; + + // Act + var fromSql = GenieMessageStateExtensions.FromWire(sqlSpelling); + var fromGenie = GenieMessageStateExtensions.FromWire(genieSpelling); + + // Assert + fromSql.ShouldBe(GenieMessageState.Unknown); + fromGenie.ShouldBe(GenieMessageState.Cancelled); } - // Published Databricks documentation shows a status of IN_PROGRESS, which appears in no - // SDK. A client that threw on unrecognised values would compile, pass its mocks, and fail - // against the real service. [Fact] public void Tolerates_a_status_that_appears_only_in_documentation() { - var state = GenieMessageStateExtensions.FromWire("IN_PROGRESS"); + // Arrange — published Databricks documentation shows IN_PROGRESS, which exists in no SDK. + // A client that threw on unrecognised values would compile, pass its mocks, and fail + // against the real service. + const string documentedButAbsentFromEverySdk = "IN_PROGRESS"; + // Act + var state = GenieMessageStateExtensions.FromWire(documentedButAbsentFromEverySdk); + + // Assert state.ShouldBe(GenieMessageState.Unknown); state.IsTerminal().ShouldBeFalse(); } @@ -78,9 +126,13 @@ public void Tolerates_a_status_that_appears_only_in_documentation() [Fact] public void Every_state_has_progress_text() { - foreach (var state in Enum.GetValues()) - { - state.ToProgressDescription().ShouldNotBeNullOrWhiteSpace(); - } + // Arrange + var states = Enum.GetValues(); + + // Act + var descriptions = states.Select(s => s.ToProgressDescription()).ToList(); + + // Assert + descriptions.ShouldAllBe(d => !string.IsNullOrWhiteSpace(d)); } } diff --git a/tests/LakeSpeak.Genie.Tests/OutputFidelityTests.cs b/tests/LakeSpeak.Genie.Tests/OutputFidelityTests.cs index b54e906..2a95837 100644 --- a/tests/LakeSpeak.Genie.Tests/OutputFidelityTests.cs +++ b/tests/LakeSpeak.Genie.Tests/OutputFidelityTests.cs @@ -1,4 +1,3 @@ -using LakeSpeak.Genie; using LakeSpeak.Rendering; namespace LakeSpeak.Genie.Tests; @@ -17,6 +16,10 @@ private static GenieQueryResult Result(params (string Name, string? Value)[] cel IsTruncated: false, TotalRowCount: 1); + private static GenieResponse Response(string? text, GenieQueryResult? result) => + new("agent", "conversation", "message", GenieMessageState.Completed, + text, null, result, [], new GenieResponseMetadata(TimeSpan.Zero, 1)); + [Theory] [InlineData("4500000.00")] [InlineData("3350000.50")] @@ -26,10 +29,14 @@ private static GenieQueryResult Result(params (string Name, string? Value)[] cel [InlineData("99999999999999999999999999.99")] public void Csv_carries_numbers_through_unchanged(string value) { - var csv = CsvWriter.Write(Result(("amount", value))); + // Arrange + var result = Result(("amount", value)); + + // Act + var csv = CsvWriter.Write(result); - // Not 4.5E6, not 4,500,000.00, not 4500000. Parsing a DECIMAL in order to print it is - // how a client silently changes someone's revenue figure. + // Assert — not 4.5E6, not 4,500,000.00, not 4500000. Parsing a DECIMAL in order to + // print it is how a client silently changes someone's revenue figure. csv.ShouldContain(value); } @@ -41,30 +48,37 @@ public void Csv_carries_numbers_through_unchanged(string value) [InlineData("emoji 🙂 in a cell")] public void Non_ascii_survives_every_writer(string value) { + // Arrange var result = Result(("label", value)); - var response = new GenieResponse( - "agent", "conversation", "message", GenieMessageState.Completed, - value, null, result, [], new GenieResponseMetadata(TimeSpan.Zero, 1)); - - CsvWriter.Write(result).ShouldContain(value); - MarkdownWriter.Write(response).ShouldContain(value); - TerminalSafety.Sanitize(value).ShouldBe(value); - - // JSON is asserted after a round trip rather than by substring. Characters outside the - // BMP are legitimately written as escaped surrogate pairs, which no consumer ever sees - // because every parser decodes them — so a substring check would fail on correct output. - using var parsed = System.Text.Json.JsonDocument.Parse(MachineOutput.ToJson(response)); - parsed.RootElement.GetProperty("answer").GetString().ShouldBe(value); + var response = Response(value, result); + + // Act + var csv = CsvWriter.Write(result); + var markdown = MarkdownWriter.Write(response); + var sanitized = TerminalSafety.Sanitize(value); + using var json = System.Text.Json.JsonDocument.Parse(MachineOutput.ToJson(response)); + + // Assert — JSON is checked after a round trip rather than by substring: characters + // outside the BMP are legitimately written as escaped surrogate pairs, which every + // parser decodes, so a substring check would fail on correct output. + csv.ShouldContain(value); + markdown.ShouldContain(value); + sanitized.ShouldBe(value); + json.RootElement.GetProperty("answer").GetString().ShouldBe(value); } - // A SQL NULL and the empty string are different values, and a format that renders them - // identically loses information the caller cannot recover. [Fact] public void Csv_distinguishes_null_from_empty_string() { - var csv = CsvWriter.Write(Result(("a", null), ("b", string.Empty))); - var dataLine = csv.Split('\n')[1].TrimEnd('\r'); + // Arrange — a SQL NULL and the empty string are different values, and a format that + // renders them identically loses information the caller cannot recover. + var result = Result(("a", null), ("b", string.Empty)); + + // Act + var csv = CsvWriter.Write(result); + // Assert + var dataLine = csv.Split('\n')[1].TrimEnd('\r'); dataLine.ShouldBe(","); } @@ -75,32 +89,59 @@ public void Csv_distinguishes_null_from_empty_string() [InlineData("@SUM(A1)")] public void Csv_defuses_spreadsheet_formulas(string value) { - var csv = CsvWriter.Write(Result(("payload", value))); + // Arrange + var result = Result(("payload", value)); - // A leading =, +, - or @ makes Excel and Sheets evaluate the cell. The value is still - // present and readable; it just cannot execute. + // Act + var csv = CsvWriter.Write(result); + + // Assert — the value is still present and readable; it just cannot execute. csv.ShouldContain($"\"'{value}\""); } + [Theory] + [InlineData("\t=cmd|' /c calc'!A1")] + [InlineData("\r=1+1")] + public void Csv_defuses_a_formula_hidden_behind_leading_whitespace(string value) + { + // Arrange — OWASP documents tab and carriage return as accepted prefixes before the + // formula marker; a guard that only checks value[0] misses both. + var result = Result(("payload", value)); + + // Act + var csv = CsvWriter.Write(result); + + // Assert + csv.ShouldContain("'"); + } + [Fact] public void Csv_quotes_values_containing_delimiters_and_quotes() { - var csv = CsvWriter.Write(Result(("a", "has,comma"), ("b", "has\"quote"))); + // Arrange + var result = Result(("a", "has,comma"), ("b", "has\"quote")); + + // Act + var csv = CsvWriter.Write(result); + // Assert csv.ShouldContain("\"has,comma\""); csv.ShouldContain("\"has\"\"quote\""); } - // Genie returns model-generated prose and cells drawn from your tables. A crafted value must - // not be able to move the cursor or draw something resembling this tool's own prompt. [Theory] [InlineData("cleared")] [InlineData("bell")] [InlineData("carriage\rreturn")] public void Control_characters_are_neutralised(string value) { + // Arrange — Genie returns model-generated prose and cells drawn from your tables. A + // crafted value must not be able to move the cursor or draw this tool's own prompt. + + // Act var sanitized = TerminalSafety.SanitizeCell(value); + // Assert sanitized.ShouldNotContain(""); sanitized.ShouldNotContain(""); sanitized.ShouldNotContain("\r"); @@ -109,28 +150,41 @@ public void Control_characters_are_neutralised(string value) [Fact] public void Newline_and_tab_survive_sanitising_prose() { + // Arrange const string prose = "line one\nline two\tcolumn"; - TerminalSafety.Sanitize(prose).ShouldBe(prose); + // Act + var sanitized = TerminalSafety.Sanitize(prose); + + // Assert + sanitized.ShouldBe(prose); } [Fact] public void Markdown_escapes_pipes_so_a_cell_cannot_break_the_table() { - var markdown = MarkdownWriter.Write(new GenieResponse( - "a", "c", "m", GenieMessageState.Completed, null, null, - Result(("col", "a|b")), [], new GenieResponseMetadata(TimeSpan.Zero, 1))); + // Arrange + var response = Response(null, Result(("col", "a|b"))); + + // Act + var markdown = MarkdownWriter.Write(response); + // Assert markdown.ShouldContain("a\\|b"); } [Fact] public void Json_uses_a_stable_lowercase_status_vocabulary() { - var json = MachineOutput.ToJson(new GenieResponse( + // Arrange + var response = new GenieResponse( "a", "c", "m", GenieMessageState.QueryResultExpired, null, null, null, [], - new GenieResponseMetadata(TimeSpan.Zero, 1))); + new GenieResponseMetadata(TimeSpan.Zero, 1)); + + // Act + var json = MachineOutput.ToJson(response); + // Assert json.ShouldContain("\"status\": \"queryresultexpired\""); json.ShouldContain("\"schemaVersion\": \"1\""); } diff --git a/tests/LakeSpeak.Genie.Tests/RedactionEvasionTests.cs b/tests/LakeSpeak.Genie.Tests/RedactionEvasionTests.cs index 3bcc1ca..19290a0 100644 --- a/tests/LakeSpeak.Genie.Tests/RedactionEvasionTests.cs +++ b/tests/LakeSpeak.Genie.Tests/RedactionEvasionTests.cs @@ -1,5 +1,3 @@ -using LakeSpeak.Genie; - namespace LakeSpeak.Genie.Tests; /// @@ -12,8 +10,7 @@ namespace LakeSpeak.Genie.Tests; /// public class RedactionEvasionTests { - // The token here is deliberately neither JWT-shaped nor dapi-shaped, so the only thing that - // can catch it is the named-key rule. + // Deliberately neither JWT-shaped nor dapi-shaped, so only the named-key rule can catch it. private const string Opaque = "sometoken_not_jwt_or_dapi_shaped_1234567890"; [Theory] @@ -24,17 +21,28 @@ public class RedactionEvasionTests [InlineData("Authorization = Bearer ")] public void A_scheme_prefixed_credential_does_not_survive(string prefix) { - var scrubbed = DiagnosticRedaction.Scrub(prefix + Opaque); + // Arrange — redacting only the word "Bearer" and leaving the credential is the failure + // this whole class exists to catch. + var diagnostic = prefix + Opaque; + + // Act + var scrubbed = DiagnosticRedaction.Scrub(diagnostic); - // Redacting only the word "Bearer" and leaving the credential is the failure this - // whole class exists to catch. + // Assert scrubbed.ShouldNotContain(Opaque); } [Fact] public void A_bearer_token_on_its_own_does_not_survive() { - DiagnosticRedaction.Scrub($"Bearer {Opaque}").ShouldNotContain(Opaque); + // Arrange + var diagnostic = $"Bearer {Opaque}"; + + // Act + var scrubbed = DiagnosticRedaction.Scrub(diagnostic); + + // Assert + scrubbed.ShouldNotContain(Opaque); } [Theory] @@ -44,17 +52,26 @@ public void A_bearer_token_on_its_own_does_not_survive() [InlineData("statement_id_signature: ")] public void Named_secrets_do_not_survive_regardless_of_separator(string prefix) { - DiagnosticRedaction.Scrub(prefix + Opaque).ShouldNotContain(Opaque); + // Arrange + var diagnostic = prefix + Opaque; + + // Act + var scrubbed = DiagnosticRedaction.Scrub(diagnostic); + + // Assert + scrubbed.ShouldNotContain(Opaque); } - // Realistic shape: a header dump, where the credential is followed by more headers. The - // scrubber must take the credential without eating the rest of the line's structure. [Fact] public void Redacts_the_credential_in_a_header_dump_without_eating_everything() { - var scrubbed = DiagnosticRedaction.Scrub( - $"Authorization: Bearer {Opaque}\nContent-Type: application/json"); + // Arrange — a realistic shape, where the credential is followed by more headers. + var diagnostic = $"Authorization: Bearer {Opaque}\nContent-Type: application/json"; + // Act + var scrubbed = DiagnosticRedaction.Scrub(diagnostic); + + // Assert scrubbed.ShouldNotContain(Opaque); scrubbed.ShouldContain("Content-Type: application/json"); } @@ -62,10 +79,13 @@ public void Redacts_the_credential_in_a_header_dump_without_eating_everything() [Fact] public void Ordinary_prose_containing_the_word_bearer_is_left_readable() { + // Arrange — over-scrubbing prose would make diagnostics useless. const string prose = "The bearer of this message is not authorized."; - // Over-scrubbing prose would make diagnostics useless; the value after the keyword is - // taken, but the sentence must stay recognisable. - DiagnosticRedaction.Scrub(prose).ShouldContain("The bearer"); + // Act + var scrubbed = DiagnosticRedaction.Scrub(prose); + + // Assert + scrubbed.ShouldContain("The bearer"); } } diff --git a/tests/LakeSpeak.LiveIntegrationTests/LiveGenieTests.cs b/tests/LakeSpeak.LiveIntegrationTests/LiveGenieTests.cs index d604811..011b5bf 100644 --- a/tests/LakeSpeak.LiveIntegrationTests/LiveGenieTests.cs +++ b/tests/LakeSpeak.LiveIntegrationTests/LiveGenieTests.cs @@ -21,9 +21,9 @@ namespace LakeSpeak.LiveIntegrationTests; /// /// /// -/// Every test here is read-only apart from , which writes a -/// NONE rating — the neutral value — so a test run does not pollute a real Agent's feedback -/// signal. Nothing creates, modifies, or deletes an Agent. +/// Every test is read-only apart from , which writes a NONE +/// rating — the neutral value — so a run does not pollute a real Agent's feedback signal. +/// Nothing creates, modifies, or deletes an Agent. /// /// [Trait("Category", "Live")] @@ -72,12 +72,16 @@ private async Task ResolveAgentAsync() [Fact] public async Task Agents_can_be_listed() { + // Arrange var agents = new List(); + + // Act await foreach (var agent in Client.ListAllAgentsAsync(Ct)) { agents.Add(agent); } + // Assert agents.ShouldNotBeEmpty(); agents.ShouldAllBe(a => a.AgentId.Length > 0); agents.ShouldAllBe(a => a.Title.Length > 0); @@ -86,33 +90,37 @@ public async Task Agents_can_be_listed() [Fact] public async Task A_question_returns_an_answer_and_the_sql_behind_it() { + // Arrange var agent = await ResolveAgentAsync(); + // Act var response = await Client.AskAsync( agent.AgentId, "How many rows are in the data?", cancellationToken: Ct); + // Assert — not asserted: that the answer is correct. No client can check that, and a + // test pretending otherwise would be the exact overreach this project's docs warn about. response.State.ShouldBe(GenieMessageState.Completed); response.ConversationId.ShouldNotBeNullOrWhiteSpace(); response.MessageId.ShouldNotBeNullOrWhiteSpace(); response.Text.ShouldNotBeNullOrWhiteSpace(); - - // Not asserted: that the answer is correct. No client can check that, and a test - // pretending otherwise would be the exact overreach this project's docs warn about. } /// /// The claim this project makes most loudly: a value is never reformatted on its way out. - /// Asserted against a live warehouse rather than a fixture, because a fixture cannot catch - /// a serialiser deciding to parse a DECIMAL somewhere in the middle. + /// Asserted against a live warehouse rather than a fixture, because a fixture cannot catch a + /// serialiser deciding to parse a DECIMAL somewhere in the middle. /// [Fact] public async Task Result_cells_arrive_as_strings_and_are_never_reformatted() { + // Arrange var agent = await ResolveAgentAsync(); + // Act var response = await Client.AskAsync( agent.AgentId, "Show every row with all columns.", cancellationToken: Ct); + // Assert if (response.Result is not { Rows.Count: > 0 } result) { // Genie may answer in prose. That is not a failure of this client. @@ -131,12 +139,15 @@ public async Task Result_cells_arrive_as_strings_and_are_never_reformatted() [Fact] public async Task A_follow_up_stays_in_the_same_conversation() { + // Arrange var agent = await ResolveAgentAsync(); - var first = await Client.AskAsync(agent.AgentId, "How many rows are there?", cancellationToken: Ct); + + // Act var second = await Client.FollowUpAsync( agent.AgentId, first.ConversationId, "And how many columns?", cancellationToken: Ct); + // Assert second.ConversationId.ShouldBe(first.ConversationId); second.MessageId.ShouldNotBe(first.MessageId); } @@ -144,32 +155,47 @@ public async Task A_follow_up_stays_in_the_same_conversation() [Fact] public async Task An_unknown_agent_id_is_reported_as_not_found() { - var ex = await Should.ThrowAsync( - () => Client.AskAsync("01f00000000000000000000000000000", "hello", cancellationToken: Ct)); + // Arrange + const string absent = "01f00000000000000000000000000000"; + + // Act + var act = () => Client.AskAsync(absent, "hello", cancellationToken: Ct); + // Assert + var ex = await Should.ThrowAsync(act); ex.Kind.ShouldBeOneOf(GenieFailureKind.AgentNotFound, GenieFailureKind.Authorization); } [Fact] public async Task Feedback_can_be_sent() { + // Arrange — NONE, not POSITIVE or NEGATIVE, so a run does not skew a real Agent's + // feedback. No comment either: Databricks rejects text alongside a NONE rating, an + // undocumented constraint this test discovered. var agent = await ResolveAgentAsync(); var response = await Client.AskAsync(agent.AgentId, "How many rows are there?", cancellationToken: Ct); - // NONE, not POSITIVE or NEGATIVE: a test run must not skew a real Agent's feedback. - // No comment either — Databricks rejects text alongside a NONE rating, which is an - // undocumented constraint this test discovered. - await Client.SendFeedbackAsync( + // Act + var act = () => Client.SendFeedbackAsync( agent.AgentId, response.ConversationId, response.MessageId, GenieFeedbackRating.None, comment: null, Ct); + + // Assert + await Should.NotThrowAsync(act); } [Fact] public async Task Feedback_text_with_a_none_rating_is_rejected_before_the_request() { - var ex = await Should.ThrowAsync(() => Client.SendFeedbackAsync( - "agent", "conversation", "message", GenieFeedbackRating.None, "a comment", Ct)); + // Arrange — caught client-side so the caller gets a clear message rather than an + // HTTP 400 that reads like a transport fault. + // Act + var act = () => Client.SendFeedbackAsync( + "agent", "conversation", "message", GenieFeedbackRating.None, "a comment", Ct); + + // Assert + var ex = await Should.ThrowAsync(act); ex.Message.ShouldContain("positive or negative"); } @@ -180,11 +206,16 @@ public async Task Feedback_text_with_a_none_rating_is_rejected_before_the_reques [Fact] public async Task Cancelling_a_question_stops_promptly() { + // Arrange var agent = await ResolveAgentAsync(); using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(500)); - await Should.ThrowAsync( - () => Client.AskAsync(agent.AgentId, "Summarise every column in detail.", cancellationToken: cts.Token)); + // Act + var act = () => Client.AskAsync( + agent.AgentId, "Summarise every column in detail.", cancellationToken: cts.Token); + + // Assert + await Should.ThrowAsync(act); } public void Dispose() => _services.Dispose(); diff --git a/tests/LakeSpeak.QuestionPacks.Tests/ConfigAliasTests.cs b/tests/LakeSpeak.QuestionPacks.Tests/ConfigAliasTests.cs index 3dc0156..ab07d0c 100644 --- a/tests/LakeSpeak.QuestionPacks.Tests/ConfigAliasTests.cs +++ b/tests/LakeSpeak.QuestionPacks.Tests/ConfigAliasTests.cs @@ -27,6 +27,7 @@ private LakeSpeakConfig Load(string yaml) [InlineData("FiNaNcE")] public void An_alias_resolves_regardless_of_case(string typed) { + // Arrange var config = Load( """ version: 1 @@ -35,16 +36,20 @@ public void An_alias_resolves_regardless_of_case(string typed) id: 01f-finance """); - config.Agents.TryGetValue(typed, out var alias).ShouldBeTrue(); + // Act + var found = config.Agents.TryGetValue(typed, out var alias); + + // Assert + found.ShouldBeTrue(); alias!.Id.ShouldBe("01f-finance"); } - // Falling through to title matching on a case mismatch is not a harmless extra round trip: - // it can select a different Agent whose title happens to match, which is the - // answer-against-the-wrong-data failure the resolver exists to prevent. [Fact] public void The_comparer_survives_deserialisation() { + // Arrange — falling through to title matching on a case mismatch is not a harmless extra + // round trip: it can select a different Agent whose title happens to match, which is the + // answer-against-the-wrong-data failure the resolver exists to prevent. var config = Load( """ version: 1 @@ -53,15 +58,24 @@ public void The_comparer_survives_deserialisation() id: 01f-sales """); - config.Agents.Comparer.ShouldBe(StringComparer.OrdinalIgnoreCase); + // Act + var comparer = config.Agents.Comparer; + + // Assert + comparer.ShouldBe(StringComparer.OrdinalIgnoreCase); } [Fact] public void A_config_with_no_agents_section_still_has_a_case_insensitive_dictionary() { + // Arrange var config = Load("version: 1"); - config.Agents.Comparer.ShouldBe(StringComparer.OrdinalIgnoreCase); + // Act + var comparer = config.Agents.Comparer; + + // Assert + comparer.ShouldBe(StringComparer.OrdinalIgnoreCase); } public void Dispose() diff --git a/tests/LakeSpeak.QuestionPacks.Tests/QuestionPackLoaderTests.cs b/tests/LakeSpeak.QuestionPacks.Tests/QuestionPackLoaderTests.cs index 3c8a41d..1c33d77 100644 --- a/tests/LakeSpeak.QuestionPacks.Tests/QuestionPackLoaderTests.cs +++ b/tests/LakeSpeak.QuestionPacks.Tests/QuestionPackLoaderTests.cs @@ -1,5 +1,3 @@ -using LakeSpeak.QuestionPacks; - namespace LakeSpeak.QuestionPacks.Tests; public class QuestionPackLoaderTests @@ -31,8 +29,12 @@ public class QuestionPackLoaderTests [Fact] public void Parses_a_valid_pack() { + // Arrange — the canonical pack above. + + // Act var pack = QuestionPackLoader.Parse(Valid, "/packs"); + // Assert pack.Name.ShouldBe("daily-brief"); pack.Agent.ShouldBe("platform-operations"); pack.Questions.Single().Id.ShouldBe("failed-jobs"); @@ -45,7 +47,8 @@ public void Parses_a_valid_pack() [Fact] public void Defaults_are_the_safe_ones() { - var pack = QuestionPackLoader.Parse( + // Arrange — a pack with no behavior block at all. + const string minimal = """ apiVersion: lakespeak.dev/v1alpha1 kind: QuestionPack @@ -56,45 +59,55 @@ public void Defaults_are_the_safe_ones() questions: - id: q1 ask: How did revenue change? - """, - "/packs"); + """; + + // Act + var pack = QuestionPackLoader.Parse(minimal, "/packs"); - // Identifiers off by default: a conversation id points at governed data, and a report - // gets committed. + // Assert — identifiers off by default because a conversation id points at governed data + // and reports get committed; continue-on-failure on, because partial results beat none + // for a scheduled report. pack.Behavior.IncludeIdentifiers.ShouldBeFalse(); pack.Behavior.IncludeGeneratedSql.ShouldBeFalse(); - // Partial results beat no results for a scheduled report. pack.Behavior.ContinueOnQuestionFailure.ShouldBeTrue(); } - // A pack can arrive from a pull request, so its output path is attacker-influenced. [Theory] [InlineData("../escaped.md")] [InlineData("../../etc/passwd")] [InlineData("sub/../../escaped.md")] public void Rejects_an_output_path_that_escapes_the_pack_directory(string path) { + // Arrange — a pack can arrive from a pull request, so its output path is + // attacker-influenced. var yaml = Valid.Replace("path: reports/daily.md", $"path: {path}", StringComparison.Ordinal); + // Act var ex = Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")); + // Assert ex.Errors.ShouldContain(e => e.Contains("outside the pack directory", StringComparison.Ordinal)); } [Fact] public void Rejects_an_absolute_output_path() { + // Arrange var absolute = OperatingSystem.IsWindows() ? @"C:\temp\out.md" : "/tmp/out.md"; var yaml = Valid.Replace("path: reports/daily.md", $"path: '{absolute}'", StringComparison.Ordinal); - Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")) - .Errors.ShouldContain(e => e.Contains("must be relative", StringComparison.Ordinal)); + // Act + var ex = Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")); + + // Assert + ex.Errors.ShouldContain(e => e.Contains("must be relative", StringComparison.Ordinal)); } [Fact] public void Reports_every_problem_at_once() { - var yaml = + // Arrange — a pack broken three different ways. + const string broken = """ apiVersion: lakespeak.dev/v1alpha1 kind: QuestionPack @@ -111,9 +124,10 @@ public void Reports_every_problem_at_once() ask: three """; - var ex = Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")); + // Act + var ex = Should.Throw(() => QuestionPackLoader.Parse(broken, "/packs")); - // Reporting only the first turns fixing a pack into repeated guessing. + // Assert — reporting only the first turns fixing a pack into repeated guessing. ex.Errors.Count.ShouldBeGreaterThanOrEqualTo(3); ex.Errors.ShouldContain(e => e.Contains("kebab-case", StringComparison.Ordinal)); ex.Errors.ShouldContain(e => e.Contains("duplicate question id", StringComparison.Ordinal)); @@ -124,27 +138,39 @@ public void Reports_every_problem_at_once() [InlineData("lakespeak.dev/v1alpha1", "kind")] public void Rejects_a_wrong_apiVersion_or_kind(string apiVersion, string broken) { + // Arrange var yaml = Valid.Replace("apiVersion: lakespeak.dev/v1alpha1", $"apiVersion: {apiVersion}", StringComparison.Ordinal); if (broken == "kind") { yaml = yaml.Replace("kind: QuestionPack", "kind: Something", StringComparison.Ordinal); } - Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")); + // Act + var act = () => QuestionPackLoader.Parse(yaml, "/packs"); + + // Assert + Should.Throw(act); } [Fact] public void Requires_an_agent() { + // Arrange — a report that silently ran against a different Agent is worse than one that + // failed, so the Agent is never inferred. var yaml = Valid.Replace(" agent: platform-operations\n", string.Empty, StringComparison.Ordinal); - Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")) - .Errors.ShouldContain(e => e.Contains("spec.agent is required", StringComparison.Ordinal)); + // Act + var ex = Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")); + + // Assert + ex.Errors.ShouldContain(e => e.Contains("spec.agent is required", StringComparison.Ordinal)); } [Fact] public void Caps_the_number_of_questions() { + // Arrange — each question occupies a SQL warehouse; beyond the cap it is a scheduled + // job, not a report. var questions = string.Join('\n', Enumerable.Range(1, 51).Select(i => $" - id: q{i}\n ask: question {i}")); @@ -160,27 +186,38 @@ public void Caps_the_number_of_questions() {questions} """; - Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")) - .Errors.ShouldContain(e => e.Contains("maximum is 50", StringComparison.Ordinal)); + // Act + var ex = Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")); + + // Assert + ex.Errors.ShouldContain(e => e.Contains("maximum is 50", StringComparison.Ordinal)); } [Fact] public void Rejects_an_unparseable_duration() { + // Arrange var yaml = Valid.Replace("timeout: 90s", "timeout: soon", StringComparison.Ordinal); - Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")) - .Errors.ShouldContain(e => e.Contains("is not a duration", StringComparison.Ordinal)); + // Act + var ex = Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")); + + // Assert + ex.Errors.ShouldContain(e => e.Contains("is not a duration", StringComparison.Ordinal)); } - // An unknown key is far more likely a typo in a real key than a deliberate extension, and - // silently ignoring it means the setting the author intended never takes effect. [Fact] public void Rejects_an_unknown_key() { + // Arrange — an unknown key is far more likely a typo in a real key than a deliberate + // extension, and ignoring it means the setting the author intended never takes effect. var yaml = Valid.Replace(" agent: platform-operations", " agent: platform-operations\n contineuOnFailure: true", StringComparison.Ordinal); - Should.Throw(() => QuestionPackLoader.Parse(yaml, "/packs")); + // Act + var act = () => QuestionPackLoader.Parse(yaml, "/packs"); + + // Assert + Should.Throw(act); } }