Commit e811550
authored
CSHARP-5930: Update replica-set discovery and reporting in tests (#1974)
See CSHARP-6008 for details about why this change is needed.
There were multiple iterations getting to this point:
## Iteration 1
RequireServer.cs
- ClusterType()/ClusterTypes(): Now use GetActualClusterType() which maps a directConnect RS member to ClusterType.ReplicaSet (instead of the misleading Standalone)
- topology/topologies case: Uses actual server type — single matches only Standalone, replicaset matches IsReplicaSetMember()
DriverTestConfiguration.cs
- IsReplicaSet(): Checks server.Type.IsReplicaSetMember() instead of cluster.Description.Type == ClusterType.ReplicaSet, so a directConnect RS is correctly identified
ReadPreferenceOnStandaloneTests.cs (from earlier)
- Uses ServerType.Standalone instead of ClusterType.Standalone to decide whether $readPreference is expected
ServerDiscoveryProseTests.cs
- Missing secondary now throws SkipException instead of a plain Exception
UnifiedTestSpecRunner.cs
- Added IsDirectConnectionToReplicaSet() helper
- ServerDiscoveryAndMonitoring: Skips logging-replicaset.json, replicaset-emit-topology-changed-before-close.json, rediscover-quickly-after-step-down.json when on directConnect RS (these require full multi-server topology discovery events)
- ServerSelection: Skips replica-set.json when on directConnect RS (same reason)
## Iteration 2
- Root cause: My GetActualClusterType() change made RequireServer.ClusterType(ClusterType.ReplicaSet) pass for a directConnect RSPrimary. This caused Connection_pool_should_not_be_cleared_when_replSetStepDown_and_GetMore to run — a test that was previously always skipped on this setup. That test calls { replSetStepDown: 30, force: true }, which makes the single-node RS step down and refuse writes for up to 30 seconds. All subsequent write operations in other tests (like DropCollection in fixture setup) then fail with MongoNotPrimaryException.
- Fix: Added RequireServer.ReplicaSetDataBearingMembers(int minimum) — a new check that counts how many data-bearing RS members the cluster has
## Iteration 3
- Applied .ReplicaSetDataBearingMembers(2) to Connection_pool_should_not_be_cleared_when_replSetStepDown_and_GetMore — the step-down test only makes sense (and is safe) with at least 2 RS members so a new primary can be elected after the step-down
## Iteration 4
- CausalConsistencyExamples.cs — Causal_Consistency_Example_3: Replaced the hardcoded "mongodb://localhost/?readPreference=secondaryPreferred" with settings built from CoreTestConfiguration.ConnectionString plus ReadPreference.SecondaryPreferred. The example intent is preserved.
- ClientSideEncryption2Examples.cs — FLE2AutomaticEncryption: Added an explicit skip when CRYPT_SHARED_LIB_PATH is not set (CSFLE/auto-encryption requires it), and fixed two new MongoClient() calls to use CoreTestConfiguration.ConnectionString.ToString() instead of the default localhost:27017.
Fix replica-set detection in tests to handle both directConnection=true and a normal connection to a single node replica set.
- Renamed IsDirectConnectionToReplicaSet() → IsSingleNodeReplicaSet() — better reflects that it covers both connection modes.
- Updated the logic: the old method required description.DirectConnection == true. The new method checks servers.Count == 1 && servers[0].Type.IsReplicaSetMember(), which is true for both:
- directConnection=true to a RS member (old case)
- replicaSet=rs0 on a single-node RS (new case)
- Updated both call sites in ServerDiscoveryAndMonitoring and ServerSelection with updated comments.
Update Causal_Consistency_Example_2 to skip because it needs two nodes
CSHARP-5930: Address review feedback on replica-set topology helpers
- GetActualClusterType: add SpinWait (10 s) in the DirectConnection path so
the server type is known before reading it, avoiding spurious Unknown
results on a fresh cluster. Throws InvalidOperationException if the
timeout expires, matching the pattern in DriverTestConfiguration.IsReplicaSet.
- IsRequirementSatisfied "topologies"/"topology" case: same SpinWait before
reading the server type from the cluster description.
- SkipNotSupportedTestCases: add optional `reason` parameter so callers can
supply a specific message instead of the generic "not supported" text.
- Single-node RS skips in UnifiedTestSpecRunner now emit messages that
explicitly mention "single-node replica set" for searchable CI logs
("does not produce full RS topology discovery events" / "does not have a
secondary").
CSHARP-5930: Address second-round review feedback
- GetActualClusterType: replace InvalidOperationException with SkipException
on spin-wait timeout, consistent with every other Require* helper that
signals a skip rather than a hard failure when the topology is not as
expected.
- SkipNotSupportedTestCases: use StartsWith(operationName + ":") when
operationName ends in ".json" so that a similarly-named spec file (e.g.
"other-logging-replicaset.json") cannot accidentally match. Non-filename
matches continue to use Contains as before.
- ServerDiscoveryProseTests: add ReplicaSetDataBearingMembers(2) to the
RequireServer chain so single-node RS is skipped at the require level.
The secondary-not-found check below it is now a hard failure, because the
require guard means we must be on a multi-node RS where a secondary is
expected.
CSHARP-5930: Address third-round review feedback
- UnifiedTestSpecRunner: add CSHARP-6008 references to the single-node RS
skip-block comments so the skips are auditable and findable.
- UnifiedTestSpecRunner: split the dual-mode SkipNotSupportedTestCases into
two clearly-named helpers: SkipFile (StartsWith match on filename, always
requires an explicit reason) and SkipNotSupportedTestCases (Contains match
on description substring, unchanged callers).
- RequireServer.GetActualClusterType: add a one-line comment explaining why
the non-direct-connection path reads Description.Type without a spin-wait.
- RequireServer.IsTopologyMatch: add comment confirming that matching any
IsReplicaSetMember() for "replicaset" is intentional per unified-spec
semantics (Primary / Secondary / Arbiter / Other / Ghost all qualify).
- DriverTestConfiguration.IsReplicaSet: add comment noting the broad RS-
member match and directing callers that need a data-bearing member to use
GetReplicaSetNumberOfDataBearingMembers instead.
More review feedback
[review-iter 1] Address review findings
[review-iter 2] Address review findings
[review-iter 3] Address review findings
[review-iter 4] Address review findings
[review-iter 5] Address review findings
[review-iter 5] Address review findings
Minor Copilot feedback
- Copilot's concern is valid: if the spin times out, cluster.Description.Servers would likely be empty or all-Unknown, the factory would return false, and the Lazy<bool> would cache that false for the rest of the process — silently un-skipping logging-replicaset.json, replicaset-emit-topology-changed-before-close.json, rediscover-quickly-after-step-down.json, and replica-set.json on real single-node RS environments.
- The fix captures the SpinWait.SpinUntil return and throws SkipException with the cluster description on timeout, mirroring the precedent in RequireServer.GetActualClusterType() at RequireServer.cs:325-328. The Lazy then caches the exception, so all subsequent calls in a broken-cluster process throw the same skip rather than spinning another 10s each.
- Fix at RequireServer.cs:256-263: short-circuits when cluster.Description.DirectConnection && minimum > 1 so each affected test skips instantly instead of paying a 10-second timeout. The original directConnection comment is reworded into the new short-circuit branch.
- Summary: ReplicaSetDataBearingMembers now no-ops on non-RS topologies instead of throwing. Effect on the three callers:
- ConnectionsSurvivePrimaryStepDownTests.cs:80 — unchanged (already gated by .ClusterType(ReplicaSet)).
- ServerDiscoveryProseTests.cs:55 — unchanged (already gated by .ClusterTypes(ReplicaSet)).
- CausalConsistencyExamples.cs:78 — now correctly runs on Sharded (where mongos handles per-shard secondary routing for ReadPreference.Secondary) while still skipping on single-node RS.
- Three changes in ServerDiscoveryProseTests.cs:62-77:
1. Wait on the right predicate: spin on Any(s => Connected && ReplicaSetSecondary) instead of ClusterState.Connected. The old check was satisfied by the primary alone and never actually waited for a secondary.
2. Longer timeout: 3s → 10s, matching the other SDAM spins in the codebase, reducing the slow-environment flakiness Copilot flagged.
3. Richer diagnostic: include clusterDescription in the failure message, so when this branch does fire it points at the real culprit (likely ReplicaSetDataBearingMembers undercounting).
- Kept Exception rather than SkipException per the iter 2 reviewer's intent — added an inline comment so a future reader doesn't try to re-skip it. The stale "this case should be skipped" line in my old commit message was just out-of-date; iter 2 explicitly reverted that.
CSHARP-5930: Address review feedback on replica-set topology helpers
Consolidate the replica-set topology helpers into DriverTestConfiguration
and remove fragile waits, per PR review.
TL;DR
- ReplicaSetDataBearingMembers now reuses
DriverTestConfiguration.GetReplicaSetNumberOfDataBearingMembers, so it
waits for the cluster to connect and then counts instead of spinning
the full 10s when the required count can never be reached.
- GetActualClusterType moved to DriverTestConfiguration and now throws
InvalidOperationException (fail, not skip) when the cluster type cannot
be determined.
- Single-node-RS detection moved to
DriverTestConfiguration.IsSingleNodeReplicaSet.
- SkipFile identifies a test case's source spec file from the
discoverer's "_fileName" value rather than parsing the composite
test-case name.
Details
ReplicaSetDataBearingMembers (RequireServer):
The previous implementation special-cased directConnection and then
spun on `Servers.Count(IsDataBearing) >= minimum`, which always ran to
the full timeout whenever the cluster simply did not have `minimum`
data-bearing members (e.g. requiring 3 on a 2-member set, or any
multi-member requirement under directConnection). It now delegates to
DriverTestConfiguration.GetReplicaSetNumberOfDataBearingMembers, which
waits for all servers to connect and returns the real count; the method
then compares once and skips immediately. The directConnection case
needs no special handling: a direct connection only ever sees its single
server, so the count tops out at 1 and the comparison skips at once.
GetActualClusterType moved to DriverTestConfiguration:
The effective-cluster-type logic (mapping a directConnection server type
to its real cluster type, with a short spin for early-startup races) now
lives alongside the other cluster helpers in DriverTestConfiguration as
an internal static taking the cluster. RequireServer keeps a thin
delegating wrapper so its call sites are unchanged. When the type cannot
be determined it now throws InvalidOperationException rather than
SkipException, matching the existing IsReplicaSet/WaitForAllServersToBe-
Connected convention: an undeterminable topology is an environment
failure that should fail the test, not silently skip it.
IsSingleNodeReplicaSet moved to DriverTestConfiguration:
The single-node-RS detection used by the unified spec runner now lives
in DriverTestConfiguration and builds on the existing IsReplicaSet spin
(which throws if the type never resolves). The runner's process-lifetime
Lazy cache is dropped; once the cluster is connected the answer is
stable and the underlying spin returns immediately, so the cache added
no value.
SkipFile uses the discoverer's _fileName:
UnifiedTestsDiscoverer already stamps each test case's shared document
with "_fileName". SkipFile now matches on that value instead of
StartsWith-parsing the composite "<file>:<description>" test-case name.
The embedded-resource typo guard is retained so a misspelled file name
throws loudly instead of silently skipping nothing.
CSHARP-5930: Remove SkipFile embedded-resource typo guard
SkipFile previously validated its fileName argument against an
assembly-wide scan of embedded .json resources (the
__embeddedJsonSpecResources Lazy) so a typo threw a precise error
instead of silently skipping nothing.
Now that SkipFile matches on the discoverer's "_fileName" value, drop
the guard and the Lazy entirely. SkipFile only runs inside
IsSingleNodeReplicaSet() blocks, so a mistyped fileName degrades to the
file's tests running on single-node RS - the exact topology they are
skipped for - which surfaces as a normal test failure rather than a
silent pass. The reflection scan and ~18 lines of guard are not worth
that marginally more precise diagnostic.
Feedback.1 parent 3a33efa commit e811550
7 files changed
Lines changed: 175 additions & 28 deletions
File tree
- tests
- MongoDB.Driver.Examples
- MongoDB.Driver.TestHelpers
- Core/XunitExtensions
- MongoDB.Driver.Tests
- Specifications
- server-discovery-and-monitoring/prose-tests
Lines changed: 4 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
75 | 75 | | |
76 | 76 | | |
77 | 77 | | |
78 | | - | |
| 78 | + | |
79 | 79 | | |
80 | 80 | | |
81 | 81 | | |
| |||
120 | 120 | | |
121 | 121 | | |
122 | 122 | | |
123 | | - | |
| 123 | + | |
| 124 | + | |
124 | 125 | | |
125 | | - | |
| 126 | + | |
126 | 127 | | |
127 | 128 | | |
128 | 129 | | |
| |||
Lines changed: 8 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
38 | 38 | | |
39 | 39 | | |
40 | 40 | | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
41 | 45 | | |
42 | 46 | | |
43 | 47 | | |
| |||
52 | 56 | | |
53 | 57 | | |
54 | 58 | | |
55 | | - | |
| 59 | + | |
56 | 60 | | |
57 | 61 | | |
58 | 62 | | |
| |||
92 | 96 | | |
93 | 97 | | |
94 | 98 | | |
95 | | - | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
96 | 102 | | |
97 | 103 | | |
98 | 104 | | |
| |||
Lines changed: 44 additions & 14 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| 22 | + | |
22 | 23 | | |
| 24 | + | |
23 | 25 | | |
24 | 26 | | |
25 | 27 | | |
| |||
67 | 69 | | |
68 | 70 | | |
69 | 71 | | |
70 | | - | |
| 72 | + | |
71 | 73 | | |
72 | 74 | | |
73 | 75 | | |
| |||
77 | 79 | | |
78 | 80 | | |
79 | 81 | | |
80 | | - | |
| 82 | + | |
81 | 83 | | |
82 | 84 | | |
83 | 85 | | |
| |||
172 | 174 | | |
173 | 175 | | |
174 | 176 | | |
175 | | - | |
| 177 | + | |
176 | 178 | | |
177 | 179 | | |
178 | 180 | | |
179 | 181 | | |
180 | 182 | | |
181 | | - | |
| 183 | + | |
182 | 184 | | |
183 | 185 | | |
184 | 186 | | |
| |||
220 | 222 | | |
221 | 223 | | |
222 | 224 | | |
223 | | - | |
224 | | - | |
| 225 | + | |
| 226 | + | |
225 | 227 | | |
226 | 228 | | |
227 | 229 | | |
| |||
239 | 241 | | |
240 | 242 | | |
241 | 243 | | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
242 | 269 | | |
243 | 270 | | |
244 | 271 | | |
| |||
285 | 312 | | |
286 | 313 | | |
287 | 314 | | |
| 315 | + | |
| 316 | + | |
288 | 317 | | |
289 | 318 | | |
290 | 319 | | |
| |||
335 | 364 | | |
336 | 365 | | |
337 | 366 | | |
338 | | - | |
339 | | - | |
340 | | - | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
341 | 371 | | |
342 | 372 | | |
343 | 373 | | |
| |||
359 | 389 | | |
360 | 390 | | |
361 | 391 | | |
362 | | - | |
| 392 | + | |
363 | 393 | | |
364 | 394 | | |
365 | 395 | | |
366 | | - | |
367 | | - | |
| 396 | + | |
| 397 | + | |
368 | 398 | | |
369 | | - | |
370 | | - | |
| 399 | + | |
| 400 | + | |
371 | 401 | | |
372 | 402 | | |
373 | 403 | | |
| |||
Lines changed: 57 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
223 | 223 | | |
224 | 224 | | |
225 | 225 | | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
226 | 229 | | |
227 | 230 | | |
228 | | - | |
229 | | - | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
230 | 235 | | |
231 | 236 | | |
232 | 237 | | |
233 | | - | |
| 238 | + | |
234 | 239 | | |
235 | 240 | | |
236 | 241 | | |
| |||
241 | 246 | | |
242 | 247 | | |
243 | 248 | | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
244 | 252 | | |
245 | 253 | | |
246 | 254 | | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
247 | 301 | | |
248 | 302 | | |
249 | 303 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
76 | 76 | | |
77 | 77 | | |
78 | 78 | | |
79 | | - | |
| 79 | + | |
80 | 80 | | |
81 | 81 | | |
82 | 82 | | |
| |||
Lines changed: 48 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
18 | 19 | | |
19 | 20 | | |
20 | 21 | | |
| |||
210 | 211 | | |
211 | 212 | | |
212 | 213 | | |
213 | | - | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
214 | 228 | | |
215 | 229 | | |
216 | 230 | | |
217 | | - | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
218 | 242 | | |
219 | 243 | | |
220 | 244 | | |
| |||
277 | 301 | | |
278 | 302 | | |
279 | 303 | | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
280 | 323 | | |
281 | 324 | | |
282 | 325 | | |
| |||
285 | 328 | | |
286 | 329 | | |
287 | 330 | | |
288 | | - | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
289 | 334 | | |
290 | 335 | | |
291 | 336 | | |
| |||
0 commit comments