dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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).
This commit is contained in:
Joseph Doherty
2026-05-20 10:49:49 -04:00
parent d9c99242a3
commit ce9d6301e3
4 changed files with 124 additions and 108 deletions
Download Patch File Download Diff File Expand all files Collapse all files

92
tests/ScadaLink.ConfigurationDatabase.Tests/Migrations/AddAuditLogTableMigrationTests.cs
View File

@@ -1,5 +1,5 @@
using Microsoft.Data.SqlClient;
using Xunit.Abstractions;
using Xunit;
namespace ScadaLink.ConfigurationDatabase.Tests.Migrations;
@@ -11,28 +11,26 @@ namespace ScadaLink.ConfigurationDatabase.Tests.Migrations;
/// indexes, and DB roles.
/// </summary>
/// <remarks>
/// Tests early-return with a clear test-output message when the
/// <see cref="MsSqlMigrationFixture"/> reports unavailable, so CI without
/// the dev MSSQL container still runs the suite green.
/// Tests use <see cref="SkippableFactAttribute"/> + <c>Skip.IfNot(...)</c> from
/// the Xunit.SkippableFact package so the runner reports them as Skipped (not
/// Passed) when MSSQL is unreachable. xunit 2.9.x does not ship a native
/// <c>Assert.Skip</c>/<c>Assert.SkipUnless</c> — those land in xunit v3 — so
/// SkippableFact is the canonical equivalent for this project. The fixture
/// applies the migration once at construction time.
/// </remarks>
public class AddAuditLogTableMigrationTests : IClassFixture<MsSqlMigrationFixture>
{
private readonly MsSqlMigrationFixture _fixture;
private readonly ITestOutputHelper _output;
public AddAuditLogTableMigrationTests(MsSqlMigrationFixture fixture, ITestOutputHelper output)
public AddAuditLogTableMigrationTests(MsSqlMigrationFixture fixture)
{
_fixture = fixture;
_output = output;
}
[Fact]
[SkippableFact]
public async Task AppliesMigration_CreatesAuditLogTable()
{
if (!await EnsureMigrationApplied())
{
return;
}
Skip.IfNot(_fixture.Available, _fixture.SkipReason);
var exists = await ScalarAsync<int>(
"SELECT COUNT(*) FROM INFORMATION_SCHEMA.TABLES " +
@@ -41,13 +39,10 @@ public class AddAuditLogTableMigrationTests : IClassFixture<MsSqlMigrationFixtur
Assert.Equal(1, exists);
}
[Fact]
[SkippableFact]
public async Task AppliesMigration_CreatesPartitionFunction_pf_AuditLog_Month()
{
if (!await EnsureMigrationApplied())
{
return;
}
Skip.IfNot(_fixture.Available, _fixture.SkipReason);
var functionExists = await ScalarAsync<int>(
"SELECT COUNT(*) FROM sys.partition_functions WHERE name = 'pf_AuditLog_Month';");
@@ -63,26 +58,20 @@ public class AddAuditLogTableMigrationTests : IClassFixture<MsSqlMigrationFixtur
$"Expected at least 24 monthly boundaries on pf_AuditLog_Month; got {boundaryCount}.");
}
[Fact]
[SkippableFact]
public async Task AppliesMigration_CreatesPartitionScheme_ps_AuditLog_Month()
{
if (!await EnsureMigrationApplied())
{
return;
}
Skip.IfNot(_fixture.Available, _fixture.SkipReason);
var schemeExists = await ScalarAsync<int>(
"SELECT COUNT(*) FROM sys.partition_schemes WHERE name = 'ps_AuditLog_Month';");
Assert.Equal(1, schemeExists);
}
[Fact]
[SkippableFact]
public async Task AppliesMigration_TableIsPartitionAligned()
{
if (!await EnsureMigrationApplied())
{
return;
}
Skip.IfNot(_fixture.Available, _fixture.SkipReason);
// The clustered (PK) index on AuditLog must live on the ps_AuditLog_Month
// partition scheme; sys.indexes.data_space_id points at the scheme.
@@ -95,13 +84,10 @@ public class AddAuditLogTableMigrationTests : IClassFixture<MsSqlMigrationFixtur
Assert.Equal("ps_AuditLog_Month", schemeName);
}
[Fact]
[SkippableFact]
public async Task AppliesMigration_CreatesFiveNamedIndexes()
{
if (!await EnsureMigrationApplied())
{
return;
}
Skip.IfNot(_fixture.Available, _fixture.SkipReason);
var expected = new[]
{
@@ -122,13 +108,10 @@ public class AddAuditLogTableMigrationTests : IClassFixture<MsSqlMigrationFixtur
}
}
[Fact]
[SkippableFact]
public async Task AppliesMigration_CreatesAuditWriterRole_WithExpectedGrants()
{
if (!await EnsureMigrationApplied())
{
return;
}
Skip.IfNot(_fixture.Available, _fixture.SkipReason);
var roleExists = await ScalarAsync<int>(
"SELECT COUNT(*) FROM sys.database_principals " +
@@ -171,13 +154,10 @@ public class AddAuditLogTableMigrationTests : IClassFixture<MsSqlMigrationFixtur
Assert.Equal(0, deleteGranted);
}
[Fact]
[SkippableFact]
public async Task AppliesMigration_CreatesAuditPurgerRole_WithExpectedGrants()
{
if (!await EnsureMigrationApplied())
{
return;
}
Skip.IfNot(_fixture.Available, _fixture.SkipReason);
var roleExists = await ScalarAsync<int>(
"SELECT COUNT(*) FROM sys.database_principals " +
@@ -203,13 +183,10 @@ public class AddAuditLogTableMigrationTests : IClassFixture<MsSqlMigrationFixtur
Assert.Equal(1, alterSchema);
}
[Fact]
[SkippableFact]
public async Task AuditWriterRole_CannotUpdateAuditLog()
{
if (!await EnsureMigrationApplied())
{
return;
}
Skip.IfNot(_fixture.Available, _fixture.SkipReason);
// Set up a dedicated user mapped to scadalink_audit_writer, then EXECUTE AS
// and attempt UPDATE — DENY UPDATE on the role must reject the statement.
@@ -249,27 +226,6 @@ public class AddAuditLogTableMigrationTests : IClassFixture<MsSqlMigrationFixtur
// --- helpers ------------------------------------------------------------
/// <summary>
/// Applies the migration to the per-fixture test database. Returns false
/// when the fixture is unavailable (no MSSQL container) — callers should
/// log + early-return so the test is reported green on a CI box without
/// the dev container.
/// </summary>
private async Task<bool> EnsureMigrationApplied()
{
if (!_fixture.Available)
{
_output.WriteLine($"[SKIP] {_fixture.SkipReason}");
return false;
}
// ApplyAuditMigrationAsync is idempotent — repeat calls within a fixture
// are a no-op after the first migration. Cheaper than re-creating the
// database per test for M1.
await _fixture.ApplyAuditMigrationAsync();
return true;
}
private async Task<T> ScalarAsync<T>(string sql)
{
await using var conn = _fixture.OpenConnection();

125
tests/ScadaLink.ConfigurationDatabase.Tests/Migrations/MsSqlMigrationFixture.cs
View File

@@ -9,20 +9,29 @@ namespace ScadaLink.ConfigurationDatabase.Tests.Migrations;
/// Creates a fresh, uniquely-named test database on the running infra/mssql
/// container, applies the EF migrations against it, and drops it on dispose.
/// When MSSQL is not reachable (CI without the container), <see cref="Available"/>
/// is set to false so each test can early-return cleanly — keeping the test
/// suite green wherever it runs.
/// is set to false and <see cref="SkipReason"/> describes why — tests pair
/// <c>[SkippableFact]</c> with <c>Skip.IfNot(_fixture.Available, _fixture.SkipReason)</c>
/// so the runner reports them as Skipped (not silently Passed).
/// </summary>
/// <remarks>
/// xUnit 2.9.x has no dynamic Skip; the early-return-after-output pattern is
/// the project convention. Tests calling <see cref="EnsureAvailableOrSkip"/>
/// receive a clear log line in the output explaining why they did not run.
/// xunit 2.9.x has no native <c>Assert.Skip</c>/<c>Assert.SkipUnless</c> (those
/// are v3); the project uses the Xunit.SkippableFact package as the canonical
/// equivalent. The fixture attempts connect + create-db + migrate once at
/// construct time. The Connect Timeout=3 in <see cref="DefaultAdminConnectionString"/>
/// makes the fixture fail fast in a no-container environment (under ~5s total)
/// instead of hanging 30s on SqlClient's default. Only connect-failure exceptions
/// (SqlException, plus the InvalidOperationException SqlClient raises from
/// OpenAsync) flip Available to false — every other exception bubbles up so a
/// real bug is not silently swallowed.
/// </remarks>
public sealed class MsSqlMigrationFixture : IDisposable
{
// Same credentials infra/mssql/setup.sql + docker-compose use. Not a committed
// production secret — this is a local dev container connection string.
// Connect Timeout=3 makes the fixture fail fast (~3s) in a no-container
// environment rather than hanging on SqlClient's default 30s connect timeout.
private const string DefaultAdminConnectionString =
"Server=localhost,1433;User Id=sa;Password=ScadaLink_Dev1#;TrustServerCertificate=true;Encrypt=false";
"Server=localhost,1433;User Id=sa;Password=ScadaLink_Dev1#;TrustServerCertificate=true;Encrypt=false;Connect Timeout=3";
private const string AdminEnvVar = "SCADALINK_MSSQL_TEST_CONN";
@@ -58,29 +67,82 @@ public sealed class MsSqlMigrationFixture : IDisposable
? DefaultAdminConnectionString
: fromEnv;
// Step 1: open the admin connection. This is the only step that may
// legitimately fail when MSSQL is absent; SqlException + the rare
// InvalidOperationException from OpenAsync are the connect-failure
// surfaces we tolerate. Everything else (CREATE DATABASE, MigrateAsync)
// is treated as a hard fixture failure once we *have* a connection.
try
{
using var connection = new SqlConnection(_adminConnectionString);
// Short timeout so the suite skips quickly in a no-container environment
// rather than hanging on SqlClient's default 30s connect timeout.
connection.Open();
try
{
connection.Open();
}
catch (SqlException ex)
{
ConnectionString = string.Empty;
Available = false;
SkipReason = $"MSSQL unavailable (connect failed: SqlException {ex.Number}: {ex.Message})";
return;
}
catch (InvalidOperationException ex)
{
ConnectionString = string.Empty;
Available = false;
SkipReason = $"MSSQL unavailable (OpenAsync threw: {ex.Message})";
return;
}
using var createCmd = connection.CreateCommand();
createCmd.CommandText = $"CREATE DATABASE [{DatabaseName}];";
createCmd.ExecuteNonQuery();
using (var createCmd = connection.CreateCommand())
{
createCmd.CommandText = $"CREATE DATABASE [{DatabaseName}];";
createCmd.ExecuteNonQuery();
}
ConnectionString = BuildPerDbConnectionString(_adminConnectionString, DatabaseName);
// Apply the EF migrations once at fixture construction so each test
// can read from a fully-migrated database without per-test setup.
// Failures here are real bugs — let them bubble.
ApplyMigrationsCore(ConnectionString, CancellationToken.None).GetAwaiter().GetResult();
Available = true;
SkipReason = string.Empty;
}
catch (Exception ex)
catch
{
// Don't fail fixture construction — the surrounding test classes
// must remain runnable on a CI box without MSSQL. Each [Fact] gates
// on Available and skips with a clear reason via test output.
ConnectionString = string.Empty;
Available = false;
SkipReason = $"MSSQL not reachable at '{RedactPassword(_adminConnectionString)}': {ex.GetType().Name}: {ex.Message}";
// Best-effort cleanup if we created the database but failed before
// setting Available — otherwise Dispose() would skip the drop.
TryDropOrphanDatabase();
throw;
}
}
private void TryDropOrphanDatabase()
{
if (string.IsNullOrEmpty(ConnectionString))
{
return;
}
try
{
SqlConnection.ClearAllPools();
using var connection = new SqlConnection(_adminConnectionString);
connection.Open();
using var cmd = connection.CreateCommand();
cmd.CommandText =
$"IF DB_ID(N'{DatabaseName}') IS NOT NULL " +
$"BEGIN " +
$" ALTER DATABASE [{DatabaseName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; " +
$" DROP DATABASE [{DatabaseName}]; " +
$"END";
cmd.ExecuteNonQuery();
}
catch
{
// Best-effort — orphan databases carry a random guid suffix.
}
}
@@ -88,14 +150,13 @@ public sealed class MsSqlMigrationFixture : IDisposable
/// Applies the EF migrations to the per-fixture test database via a freshly
/// constructed <see cref="ScadaLinkDbContext"/> pointed at it. Uses the
/// schema-only single-argument constructor — the AuditLog migration does
/// not write secret-bearing columns at apply time.
/// not write secret-bearing columns at apply time. Called once from the
/// constructor; tests do not invoke this directly.
/// </summary>
public async Task ApplyAuditMigrationAsync(CancellationToken cancellationToken = default)
private static async Task ApplyMigrationsCore(string connectionString, CancellationToken cancellationToken)
{
ThrowIfUnavailable();
var options = new DbContextOptionsBuilder<ScadaLinkDbContext>()
.UseSqlServer(ConnectionString)
.UseSqlServer(connectionString)
.Options;
await using var context = new ScadaLinkDbContext(options);
@@ -173,20 +234,4 @@ public sealed class MsSqlMigrationFixture : IDisposable
};
return builder.ConnectionString;
}
private static string RedactPassword(string connectionString)
{
try
{
var builder = new SqlConnectionStringBuilder(connectionString)
{
Password = "***",
};
return builder.ConnectionString;
}
catch
{
return "<unparseable>";
}
}
}

7
tests/ScadaLink.ConfigurationDatabase.Tests/ScadaLink.ConfigurationDatabase.Tests.csproj
View File

@@ -26,6 +26,13 @@
<PackageReference Include="Microsoft.NET.Test.Sdk" />
<PackageReference Include="xunit" />
<PackageReference Include="xunit.runner.visualstudio" />
<!--
SkippableFact lets the Bundle C MSSQL integration tests report as Skipped
(not Passed) when the dev MSSQL container is not running. xunit 2.9.x does
not ship Assert.Skip / SkipUnless — those are v3-only — so we use the
canonical community wrapper instead.
-->
<PackageReference Include="Xunit.SkippableFact" />
</ItemGroup>
<ItemGroup>