Skip to content

Commit 4092385

Browse files
gladjohnCopilot
andcommitted
Address PR review feedback
- Fix Assert.DoesNotContain parameter order in Base64UrlHelpersExtendedTests - Replace Thread.Sleep with deterministic ThrottlingCacheEntry 3-arg ctor - Fix AddAndCleanup_SameKey_KeepsOlderIfNewer to actually test the scenario - Use cleanup interval of 0ms instead of Thread.Sleep for cleanup test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ca3a141 commit 4092385

File tree

2 files changed

+29
-20
lines changed

2 files changed

+29
-20
lines changed

tests/Microsoft.Identity.Test.Unit/Throttling/ThrottlingCacheExtendedTests.cs

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44

55
using System;
6-
using System.Threading;
76
using Microsoft.Identity.Client;
87
using Microsoft.Identity.Client.Core;
98
using Microsoft.Identity.Client.OAuth2.Throttling;
@@ -60,11 +59,15 @@ public void TryGetOrRemoveExpired_MissingKey_ReturnsFalse()
6059
public void AddAndCleanup_SameKey_KeepsNewerEntry()
6160
{
6261
var cache = new ThrottlingCache();
63-
var olderEntry = new ThrottlingCacheEntry(_ex1, TimeSpan.FromMinutes(10));
62+
var olderEntry = new ThrottlingCacheEntry(
63+
_ex1,
64+
DateTimeOffset.UtcNow.Subtract(TimeSpan.FromMinutes(1)),
65+
DateTimeOffset.UtcNow.AddMinutes(10));
6466

65-
// Small delay to ensure different creation times
66-
Thread.Sleep(10);
67-
var newerEntry = new ThrottlingCacheEntry(_ex2, TimeSpan.FromMinutes(10));
67+
var newerEntry = new ThrottlingCacheEntry(
68+
_ex2,
69+
DateTimeOffset.UtcNow,
70+
DateTimeOffset.UtcNow.AddMinutes(10));
6871

6972
cache.AddAndCleanup("k1", olderEntry, _logger);
7073
cache.AddAndCleanup("k1", newerEntry, _logger);
@@ -74,18 +77,25 @@ public void AddAndCleanup_SameKey_KeepsNewerEntry()
7477
}
7578

7679
[TestMethod]
77-
public void AddAndCleanup_SameKey_KeepsOlderIfNewer()
80+
public void AddAndCleanup_SameKey_OlderEntryDoesNotReplaceNewer()
7881
{
7982
var cache = new ThrottlingCache();
80-
var newerEntry = new ThrottlingCacheEntry(_ex1, TimeSpan.FromMinutes(10));
83+
var newerEntry = new ThrottlingCacheEntry(
84+
_ex1,
85+
DateTimeOffset.UtcNow,
86+
DateTimeOffset.UtcNow.AddMinutes(10));
8187

82-
// Add newer first, then older
88+
var olderEntry = new ThrottlingCacheEntry(
89+
_ex2,
90+
DateTimeOffset.UtcNow.Subtract(TimeSpan.FromMinutes(1)),
91+
DateTimeOffset.UtcNow.AddMinutes(10));
92+
93+
// Add newer first, then try to replace with older
8394
cache.AddAndCleanup("k1", newerEntry, _logger);
95+
cache.AddAndCleanup("k1", olderEntry, _logger);
8496

85-
// Create an entry with an older creation time by adding an already-expired entry replacement
86-
// The AddOrUpdate will keep the newer one
8797
cache.TryGetOrRemoveExpired("k1", _logger, out var foundEx);
88-
Assert.AreSame(_ex1, foundEx);
98+
Assert.AreSame(_ex1, foundEx, "Should keep the newer entry when an older entry is added later");
8999
}
90100

91101
[TestMethod]
@@ -102,17 +112,16 @@ public void CacheForTest_ExposesInternalDictionary()
102112
[TestMethod]
103113
public void Cleanup_RemovesOnlyExpiredEntries()
104114
{
105-
// Use a very short cleanup interval so cleanup triggers
106-
var cache = new ThrottlingCache(1); // 1ms cleanup interval
115+
// Use a cleanup interval of 0 so cleanup triggers immediately on next add
116+
var cache = new ThrottlingCache(0);
107117

108118
var expiredEntry = new ThrottlingCacheEntry(_ex1, TimeSpan.FromMilliseconds(-10000));
109119
var validEntry = new ThrottlingCacheEntry(_ex2, TimeSpan.FromMinutes(10));
110120

111121
cache.AddAndCleanup("expired", expiredEntry, _logger);
112122
cache.AddAndCleanup("valid", validEntry, _logger);
113123

114-
// Wait for cleanup interval to pass, then trigger cleanup via another add
115-
Thread.Sleep(50);
124+
// Trigger cleanup via another add (interval is 0ms so it fires immediately)
116125
cache.AddAndCleanup("trigger", new ThrottlingCacheEntry(_ex2, TimeSpan.FromMinutes(10)), _logger);
117126

118127
Assert.IsFalse(cache.TryGetOrRemoveExpired("expired", _logger, out _), "Expired entry should be cleaned up");

tests/Microsoft.Identity.Test.Unit/UtilTests/Base64UrlHelpersExtendedTests.cs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,9 @@ public void Encode_SimpleString_NoSpecialChars()
4444
string result = Base64UrlHelpers.Encode("Hello");
4545
Assert.IsNotNull(result);
4646
// Should not contain +, /, or =
47-
Assert.DoesNotContain(result, "+");
48-
Assert.DoesNotContain(result, "/");
49-
Assert.DoesNotContain(result, "=");
47+
Assert.DoesNotContain("+", result);
48+
Assert.DoesNotContain("/", result);
49+
Assert.DoesNotContain("=", result);
5050
}
5151

5252
[TestMethod]
@@ -101,8 +101,8 @@ public void Encode_BytesThatProducePlusAndSlash()
101101
// 0xFB, 0xEF, 0xBE => produces + and / in standard base64
102102
byte[] data = { 0xFB, 0xEF, 0xBE };
103103
string encoded = Base64UrlHelpers.Encode(data);
104-
Assert.DoesNotContain(encoded, "+");
105-
Assert.DoesNotContain(encoded, "/");
104+
Assert.DoesNotContain("+", encoded);
105+
Assert.DoesNotContain("/", encoded);
106106

107107
// Verify round trip
108108
byte[] decoded = Base64UrlHelpers.DecodeBytes(encoded);

0 commit comments

Comments
 (0)