Joseph Doherty ce9d6301e3 fix(configdb-tests): use Assert.SkipUnless + fast-fail connect timeout for MSSQL fixture (#23) ...

Reviewer of Bundle C (#23 M1) flagged two blockers in the
AddAuditLogTableMigration integration tests:

1. Tests used 'if (!await EnsureMigrationApplied()) return;' which made
   the xunit runner report them as Passed when the dev MSSQL container
   was absent — a CI false-positive risk. xunit 2.9.x does NOT ship the
   v3 Assert.Skip/SkipUnless/SkipWhen API surface (verified empirically
   against xunit.assert 2.9.3 — only v3.x exposes those static methods),
   so the canonical xunit-v2 equivalent is the Xunit.SkippableFact
   package. Replaced [Fact] with [SkippableFact] and the early-return
   pattern with 'Skip.IfNot(_fixture.Available, _fixture.SkipReason)' as
   the first statement of each of the 8 audit-log test methods. The
   runner now reports them as Skipped (not Passed) when MSSQL is down.

2. MsSqlMigrationFixture relied on SqlClient's 30s default connect
   timeout, so a no-container fixture construction hung ~30s. Added
   'Connect Timeout=3' to DefaultAdminConnectionString. Verified
   fail-fast under ~4s end-to-end with a bad host via env-var override.

Additional fixture cleanups:
- Migration is now applied once in the fixture constructor (was per-test
  via EnsureMigrationApplied for idempotency). Tests reach a fully-
  migrated database with no extra setup. Removed the now-unused
  EnsureMigrationApplied helper from the test class.
- Constructor narrowed its catch to SqlException + InvalidOperationException
  for the OpenAsync step (the only legitimate connect-failure surfaces);
  everything else (CREATE DATABASE, MigrateAsync) is treated as a hard
  fixture failure and bubbles up. Added a best-effort
  TryDropOrphanDatabase() pre-throw cleanup so partial construction
  cannot leak guid-suffixed databases.
- Stale doc comments referencing the (non-existent) xunit 2.9.x Skip
  shim removed; replaced with accurate notes about Xunit.SkippableFact.

Verified:
- dotnet build ScadaLink.slnx: clean (0 warnings, 0 errors).
- dotnet test ScadaLink.ConfigurationDatabase.Tests with MSSQL up:
  Passed 150 / Skipped 0 / Failed 0.
- Same suite with SCADALINK_MSSQL_TEST_CONN pointed at a closed port:
  the 8 AddAuditLogTableMigration tests report as Skipped (visible
  '[SKIP]' lines in runner output), total elapsed ~3s.

Files touched:
- Directory.Packages.props: added Xunit.SkippableFact 1.5.61.
- tests/ScadaLink.ConfigurationDatabase.Tests/ScadaLink.ConfigurationDatabase.Tests.csproj:
  added the SkippableFact PackageReference.
- tests/ScadaLink.ConfigurationDatabase.Tests/Migrations/MsSqlMigrationFixture.cs:
  Connect Timeout=3, constructor refactor, doc-comment fixes.
- tests/ScadaLink.ConfigurationDatabase.Tests/Migrations/AddAuditLogTableMigrationTests.cs:
  [SkippableFact] + Skip.IfNot pattern across all 8 tests.

Untouched (per reviewer guidance):
- Migration file (Bundle C main artifact unchanged).
- Bundle B reconciliation (composite PK + UX_AuditLog_EventId).
- SqlClient VersionOverride 6.1.1 in the test csproj.
- infra/* (separate uncommitted local edits remain in working tree).

2026-05-20 10:49:49 -04:00

9.3 KiB

Raw Blame History

View Raw