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).
238 lines
9.3 KiB
C#
238 lines
9.3 KiB
C#
using Microsoft.Data.SqlClient;
|
|
using Microsoft.EntityFrameworkCore;
|
|
|
|
namespace ScadaLink.ConfigurationDatabase.Tests.Migrations;
|
|
|
|
/// <summary>
|
|
/// Per-test-class MSSQL fixture for the Bundle C integration tests (#23 M1).
|
|
///
|
|
/// 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 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 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;Connect Timeout=3";
|
|
|
|
private const string AdminEnvVar = "SCADALINK_MSSQL_TEST_CONN";
|
|
|
|
public string DatabaseName { get; }
|
|
|
|
public string ConnectionString { get; }
|
|
|
|
/// <summary>
|
|
/// True when the MSSQL container was reachable at fixture construction
|
|
/// time AND the per-fixture test database was successfully created. When
|
|
/// false, the integration tests using this fixture must early-return.
|
|
/// </summary>
|
|
public bool Available { get; }
|
|
|
|
/// <summary>
|
|
/// Populated when <see cref="Available"/> is false; describes why the
|
|
/// fixture chose to skip (env var unset, connect failed, etc.).
|
|
/// </summary>
|
|
public string SkipReason { get; }
|
|
|
|
private readonly string _adminConnectionString;
|
|
|
|
public MsSqlMigrationFixture()
|
|
{
|
|
// Short, lowercase guid suffix keeps the database identifier under SQL Server's
|
|
// 128-char limit and safe for raw concatenation (no quoting required).
|
|
DatabaseName = $"ScadaLinkAuditMigTest_{Guid.NewGuid():N}".Substring(0, 38);
|
|
|
|
// Env var lets CI / power users override the admin endpoint; absent
|
|
// defaults to the local docker dev container's sa connection.
|
|
var fromEnv = Environment.GetEnvironmentVariable(AdminEnvVar);
|
|
_adminConnectionString = string.IsNullOrWhiteSpace(fromEnv)
|
|
? 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);
|
|
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();
|
|
}
|
|
|
|
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
|
|
{
|
|
// 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.
|
|
}
|
|
}
|
|
|
|
/// <summary>
|
|
/// 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. Called once from the
|
|
/// constructor; tests do not invoke this directly.
|
|
/// </summary>
|
|
private static async Task ApplyMigrationsCore(string connectionString, CancellationToken cancellationToken)
|
|
{
|
|
var options = new DbContextOptionsBuilder<ScadaLinkDbContext>()
|
|
.UseSqlServer(connectionString)
|
|
.Options;
|
|
|
|
await using var context = new ScadaLinkDbContext(options);
|
|
await context.Database.MigrateAsync(cancellationToken);
|
|
}
|
|
|
|
/// <summary>
|
|
/// Convenience for opening a fresh <see cref="SqlConnection"/> to the test
|
|
/// database. Caller is responsible for disposal.
|
|
/// </summary>
|
|
public SqlConnection OpenConnection()
|
|
{
|
|
ThrowIfUnavailable();
|
|
|
|
var connection = new SqlConnection(ConnectionString);
|
|
connection.Open();
|
|
return connection;
|
|
}
|
|
|
|
public void Dispose()
|
|
{
|
|
if (!Available)
|
|
{
|
|
return;
|
|
}
|
|
|
|
// Best-effort drop — never let a teardown failure pollute later runs.
|
|
// SINGLE_USER WITH ROLLBACK IMMEDIATE detaches lingering pooled connections
|
|
// so the DROP DATABASE doesn't fail with "database is in use".
|
|
try
|
|
{
|
|
// Connection-pool cleanup is necessary because EF's MigrateAsync leaves
|
|
// pooled connections behind; SqlConnection.ClearAllPools() forces them
|
|
// closed so the SINGLE_USER + DROP sequence below can complete.
|
|
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
|
|
{
|
|
// Swallow — the database name carries a random guid suffix so a
|
|
// stranded test database does not collide with future runs.
|
|
}
|
|
}
|
|
|
|
/// <summary>
|
|
/// Throws an <see cref="InvalidOperationException"/> when invoked on an
|
|
/// unavailable fixture; tests should branch on <see cref="Available"/>
|
|
/// before reaching this code path.
|
|
/// </summary>
|
|
private void ThrowIfUnavailable()
|
|
{
|
|
if (!Available)
|
|
{
|
|
throw new InvalidOperationException(
|
|
$"MsSqlMigrationFixture is not Available: {SkipReason}");
|
|
}
|
|
}
|
|
|
|
private static string BuildPerDbConnectionString(string adminConnectionString, string databaseName)
|
|
{
|
|
var builder = new SqlConnectionStringBuilder(adminConnectionString)
|
|
{
|
|
InitialCatalog = databaseName,
|
|
};
|
|
return builder.ConnectionString;
|
|
}
|
|
}
|