diff --git a/CHANGELOG.md b/CHANGELOG.md index 22a1ada..344daca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Six Keychain tests could read the store before their writes were visible.** `main` went + red on `macos-14` with `GetProviderNamesAsync_ReturnsDistinctProviders` reporting + *"Expected: 2, Actual: 0"* — the same commit having passed that leg minutes earlier on the + PR. `AddCredentialAsync` confirms visibility through an exact service+account query, while + `ListCredentialsAsync` and `GetProviderNamesAsync` go through the class-wide query, so one + being visible does not imply the other. Fourteen reads in that file already used + `RetryHelper`; these six did not. Each now retries on the condition it actually asserts, + not merely on something having appeared — the distinction #61 turned on. + ### Changed - **CI now runs three macOS versions and splits the libsecret tests into their own job** diff --git a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs index ed4083a..c8a89b5 100644 --- a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs +++ b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs @@ -148,7 +148,11 @@ public async Task ListCredentialsAsync_ReturnsAddedCredential() var manager = NewManager(); var accountId = await manager.AddCredentialAsync("Adobe", "prod", "Production", "{}"); - var list = (await manager.ListCredentialsAsync("Adobe")).ToList(); + // Retried like its siblings: AddCredentialAsync confirms visibility through an + // exact service+account query, while these reads go through the class-wide + // query, so one being visible does not imply the other (#61's pattern). + var list = (await RetryHelper.UntilAsync( + () => manager.ListCredentialsAsync("Adobe"), r => r.Any(c => c.AccountId == accountId))).ToList(); var credential = Assert.Single(list); Assert.Equal(accountId, credential.AccountId); @@ -167,8 +171,13 @@ public async Task ListCredentialsAsync_FiltersByProvider() _ = await manager.AddCredentialAsync("Adobe", "a1", "Production", "{}"); _ = await manager.AddCredentialAsync("Airtable", "b1", "Production", "{}"); - var adobe = (await manager.ListCredentialsAsync("Adobe")).ToList(); - var airtable = (await manager.ListCredentialsAsync("Airtable")).ToList(); + // Retried like its siblings: AddCredentialAsync confirms visibility through an + // exact service+account query, while these reads go through the class-wide + // query, so one being visible does not imply the other (#61's pattern). + var adobe = (await RetryHelper.UntilAsync( + () => manager.ListCredentialsAsync("Adobe"), r => r.Count() == 1)).ToList(); + var airtable = (await RetryHelper.UntilAsync( + () => manager.ListCredentialsAsync("Airtable"), r => r.Count() == 1)).ToList(); var adobeCredential = Assert.Single(adobe); var airtableCredential = Assert.Single(airtable); @@ -255,7 +264,12 @@ public async Task GetCredentialByIdAsync_DoesNotMutateSelection() _ = await manager.GetCredentialByIdAsync("Adobe", otherId); - var listings = (await manager.ListCredentialsAsync("Adobe")).ToList(); + // Retried like its siblings: AddCredentialAsync confirms visibility through an + // exact service+account query, while these reads go through the class-wide + // query, so one being visible does not imply the other (#61's pattern). + var listings = (await RetryHelper.UntilAsync( + () => manager.ListCredentialsAsync("Adobe"), + r => r.Count() == 2 && r.Any(c => c.IsSelected))).ToList(); Assert.True(listings.Single(c => c.AccountId == selectedId).IsSelected); Assert.False(listings.Single(c => c.AccountId == otherId).IsSelected); } @@ -282,7 +296,11 @@ public async Task DeleteCredentialAsync_RemovesCredential() var accountId = await manager.AddCredentialAsync("Adobe", "prod", "Production", "{}"); var deleted = await RetryHelper.UntilTrueAsync(() => manager.DeleteCredentialAsync(accountId)); - var list = (await manager.ListCredentialsAsync("Adobe")).ToList(); + // Retried like its siblings: AddCredentialAsync confirms visibility through an + // exact service+account query, while these reads go through the class-wide + // query, so one being visible does not imply the other (#61's pattern). + var list = (await RetryHelper.UntilAsync( + () => manager.ListCredentialsAsync("Adobe"), r => !r.Any())).ToList(); Assert.True(deleted); Assert.Empty(list); @@ -334,7 +352,12 @@ public async Task GetProviderNamesAsync_ReturnsDistinctProviders() _ = await manager.AddCredentialAsync("Adobe", "a2", "Sandbox", "{}"); _ = await manager.AddCredentialAsync("Airtable", "b1", "Production", "{}"); - var names = (await manager.GetProviderNamesAsync()).ToList(); + // This is the read that actually failed on macos-14, "Expected: 2, Actual: 0", + // and the one most exposed to the lag: GetProviderNamesAsync goes through the + // class-wide query while the adds confirmed themselves through an exact + // service+account query. + var names = (await RetryHelper.UntilAsync( + () => manager.GetProviderNamesAsync(), r => r.Count() == 2)).ToList(); Assert.Equal(2, names.Count); Assert.Contains("Adobe", names); @@ -415,7 +438,11 @@ public async Task ListCredentialsAsync_IncludesDisplayFields_FromSummaryProvider var manager = NewManager([summaryProvider]); _ = await manager.AddCredentialAsync("Adobe", "prod", "Production", "{\"apiKey\":\"xyz\"}"); - var list = (await manager.ListCredentialsAsync("Adobe")).ToList(); + // Retried like its siblings: AddCredentialAsync confirms visibility through an + // exact service+account query, while these reads go through the class-wide + // query, so one being visible does not imply the other (#61's pattern). + var list = (await RetryHelper.UntilAsync( + () => manager.ListCredentialsAsync("Adobe"), r => r.Count() == 1)).ToList(); var credential = Assert.Single(list); var field = Assert.Single(credential.DisplayFields);