From 0e39dba415f8a9accfce5c22d72ff997e97aefa3 Mon Sep 17 00:00:00 2001 From: aarroyo Date: Mon, 10 Aug 2026 12:39:31 -0500 Subject: [PATCH] =?UTF-8?q?fix(test):=20aislar=20el=20almac=C3=A9n=20de=20?= =?UTF-8?q?cada=20clase=20de=20integraci=C3=B3n=20y=20cerrar=20TD-011?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit La factoría registraba sus tres almacenes InMemory con nombres LITERALES —«TestDb», «TestProjectionDb», «TestReadModelDb»— y el proveedor InMemory de EF Core indexa por nombre dentro del proceso. Daba igual cuántas instancias de factoría hubiera: las 31 clases de la suite compartían exactamente los mismos tres almacenes, así que una escritura en cualquiera se veía desde todas. Cada instancia sufija ahora sus nombres con un GUID propio. Cada clase arranca de la misma semilla y no puede alcanzar a ninguna otra. Coste: ~5% de reloj (4m30s → 4m46s), por sembrar una vez por clase en lugar de una vez. La prueba que provocó los 133 fallos se REAÑADE a propósito, como centinela: ejercita leer-tras-escribir contra el inquilino sembrado, de modo que si alguien restituye el almacén compartido vuelve a fallar de inmediato en vez de corromper en silencio cien aserciones ajenas. No cubierto: ContractTestWebApplicationFactory y PostgreSqlWebApplicationFactory usan el mismo patrón de nombre fijo. Ninguna da síntoma hoy —la de contrato es una sola colección y la de PostgreSQL se salta sin Docker— pero el riesgo es idéntico y queda anotado. ## TD-003 se cierra: ya estaba implementado La entrada declaraba pendiente «migrar la caché a Redis sin cambiar IConfigurationProvider». Esa migración existe: RedisConfigurationCache son 291 líneas con invalidación por Pub/Sub entre réplicas y sin un solo NotImplementedException, la abstracción IConfigurationCache está en Ums.Application, el registro elige entre Redis y memoria según `Redis:Connection` —leyendo también la forma de Kubernetes— y hay pruebas de invalidación entre réplicas. Todas las mitigaciones que listaba como trabajo futuro están puestas. Lo que queda es aprovisionar Redis, que es infraestructura, no deuda de este repositorio. Nadie la releyó al aterrizar el trabajo. Un registro de deuda solo sirve si se cierra lo pagado; arrastrar un fantasma es la misma forma de fallo que mostró TD-002. Verificado: Integración 314/316 (2 saltadas) en Release con la centinela dentro; build 0 errores. Co-Authored-By: Claude Opus 5 --- docs/architecture/technical-debt.md | 54 ++++++++++--------- .../Identity/TenantBrandingRestQueryTests.cs | 47 +++++++++++++--- .../UmsApiWebApplicationFactory.cs | 22 ++++++-- 3 files changed, 88 insertions(+), 35 deletions(-) diff --git a/docs/architecture/technical-debt.md b/docs/architecture/technical-debt.md index fb85f444..f57062a2 100644 --- a/docs/architecture/technical-debt.md +++ b/docs/architecture/technical-debt.md @@ -43,25 +43,21 @@ The entry had already been symptomless for months: the 2026-08-09 resync withdre ## [TD-003] Prepare Configuration System for Redis Migration -- **Status**: Proposed -- **Severity**: Low -- **Component**: `Ums.Infrastructure/Configuration/`, `Ums.Application/Configuration/` -- **Description**: The parameterization system already runs on an in-memory cache abstraction. The remaining debt is to migrate the cache implementation to Redis without changing the business-facing `IConfigurationProvider` or the typed `ConfigurationValues` consumers. -- **Rationale**: Initial implementation uses in-memory storage for simplicity and fast iteration. Redis will be needed when UMS scales to multiple API instances requiring shared configuration state and shared cache invalidation. -- **Impact**: - - Current in-memory implementation may show stale data if multiple API instances have different cache states. - - No distributed cache invalidation across pods. - - Parameter changes still depend on the current reload path rather than a distributed cache event. -- **Mitigation**: - - Introduce `IConfigurationCache` abstraction from day one. - - Implement `InMemoryConfigurationCache` as the initial concrete implementation. - - Design `ConfigurationProvider` to depend on `IConfigurationCache`, not on concrete implementation. - - Use `ConfigurationValues` for strongly-typed consumers so the migration does not leak cache concerns into handlers or validators. - - Document the abstraction interface and future migration steps. -- **Target Resolution**: Phase 2 (when scaling to multiple API instances or when Redis infrastructure is available). -- **Related Documents**: - - [Parameterization System Specification](../governance/construction/ddd-design/parameterization-system-spec.md) - - [TODO-003](../governance/project/TODO.md#todo-003-implement-parameterization-system-with-loader-and-provider) +- **Status**: **Closed — already implemented** (verified 2026-08-10) +- **Severity**: was Low +- **Description**: The entry described the remaining work as "migrate the cache implementation to Redis without changing the business-facing `IConfigurationProvider`". That migration exists and is wired. + +### What was found on inspection + +- `Ums.Application/Configuration/Services/IConfigurationCache.cs` — the abstraction the entry proposed introducing. +- `Ums.Infrastructure/Configuration/RedisConfigurationCache.cs` — 291 lines, Redis-backed with Pub/Sub invalidation across replicas, no `NotImplementedException`. +- `Ums.Infrastructure/Configuration/InMemoryConfigurationCache.cs` — the in-process fallback. +- `DependencyInjection.cs` selects between them on `Redis:Connection` (or `REDIS_CONNECTION`, the Kubernetes form — reading only the former is a bug that was already found and fixed, see `CadenaDeRedis.Normalizar`). +- `Ums.Application.Test/Configuration/AvisoDeConfiguracionEntreReplicasTests.cs` covers cross-replica invalidation. + +Every mitigation the entry listed as future work is in place, and the "impact" it describes — stale data across instances, no distributed invalidation — does not apply when Redis is configured. What remains is **provisioning Redis in an environment**, which is infrastructure, not debt in this repository. + +- **Why it stayed open**: nobody re-read it after the work landed. A debt register is only useful if entries are closed when the debt is paid; carrying a phantom is the failure mode TD-002 also showed. --- @@ -226,13 +222,19 @@ Fixed here rather than filed separately, because otherwise TD-008's verification ## [TD-011] The Integration Suite Shares One Host Across 31 Test Classes -- **Status**: Confirmed +- **Status**: **Resolved** (2026-08-10) - **Severity**: High - **Component**: `src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs` -- **Description**: 31 test classes take `IClassFixture` over a shared seeded store. Any test that **writes** mutates state every other class reads. There is no per-test isolation and no cleanup. -- **How it surfaced (2026-08-10)**: a single new test that wrote branding to the first seeded tenant and read it back took **133 of 316 tests down in CI**. It passed locally, on both Debug and Release — execution order happened to hide it. One added write, 133 failures, and a green local run: that is the failure mode this design produces. -- **Not the first time**: the factory's own comments record an earlier incident of roughly 80 failures caused by a static constructor leaking `Persistence__Provider` between hosts. The mechanism differs; the shape does not. -- **Impact**: the suite cannot cover any write path end to end without risking the rest of it, so write coverage is pushed down to handler tests. That is a reasonable place for it, but it should be a choice, not a constraint. It also means a genuine regression can hide behind an unrelated test's pollution, and that failures do not reproduce locally — the most expensive kind. -- **Suggested resolution**: give each test class its own store (a per-class database name is the cheapest step), or add a reset between classes. The e2e suite already solved the equivalent problem for the shared cluster with `tests/helpers/limpieza.ts` and a per-run marker; the same reasoning applies here. -- **Interim measure**: `TenantBrandingRestQueryTests` covers only the read path. Read-after-write for branding is deliberately absent, with the reason written in the file so nobody re-adds it and repeats the 133. +- **Description**: 31 test classes take `IClassFixture`. The factory registered its three EF Core InMemory stores under **fixed literal names** — `"TestDb"`, `"TestProjectionDb"`, `"TestReadModelDb"` — and the InMemory provider keys a store by name **per process**. Every class therefore shared the same three stores no matter how many factory instances existed. Any write in one class was visible to all the others. +- **How it surfaced**: a single new test that wrote branding to the first seeded tenant and read it back took **133 of 316 tests down in CI**. It passed locally on both Debug and Release — execution order hid it. One added write, 133 failures, and a green local run. +- **Not the first time**: the factory's own comments record an earlier incident of roughly 80 failures from a static constructor leaking `Persistence__Provider` between hosts (G-014). Different mechanism, same shape. + +### Resolution + +Each factory instance now suffixes its three store names with a per-instance GUID, so every test class gets its own store, seeded from the same baseline and unable to reach any other. Cost: ~5% wall-clock (4m30s → 4m46s), from seeding per class instead of once. + +The write test that caused the 133 is **kept, deliberately, as a sentinel**: it exercises read-after-write against the shared-by-default seeded tenant, so if anyone reinstates a shared store it fails again immediately rather than silently corrupting a hundred unrelated assertions. + +- **Verified**: 314/316 (2 skipped) in Release with the sentinel in place. +- **Not covered by this fix**: `ContractTestWebApplicationFactory` and `PostgreSqlWebApplicationFactory` use the same literal-name pattern. Neither showed a symptom — the contract suite is a single collection and the PostgreSQL one is skipped without Docker — but the hazard is identical and worth the same treatment when either grows. diff --git a/src/apps/ums.api/Ums.Presentation.IntegrationTest/Identity/TenantBrandingRestQueryTests.cs b/src/apps/ums.api/Ums.Presentation.IntegrationTest/Identity/TenantBrandingRestQueryTests.cs index 3bac5009..00c5a853 100644 --- a/src/apps/ums.api/Ums.Presentation.IntegrationTest/Identity/TenantBrandingRestQueryTests.cs +++ b/src/apps/ums.api/Ums.Presentation.IntegrationTest/Identity/TenantBrandingRestQueryTests.cs @@ -12,12 +12,11 @@ namespace Ums.Presentation.IntegrationTest.Identity; /// Se fijan los dos desenlaces que el handler distingue en LECTURA, porque son los que el /// panel de identidad visual necesita separar y los que un 404 indiscriminado borraría. /// -/// NO se prueba aquí «leer lo que el POST acaba de escribir». Se intentó y hundió 133 -/// pruebas en CI: las 31 clases de esta suite comparten `UmsApiWebApplicationFactory`, así que -/// escribir sobre el inquilino sembrado contamina a todas las demás. En local pasaba —el orden de -/// ejecución lo escondía—, que es la peor forma de que un fallo así se manifieste. El camino de -/// escritura ya está cubierto en `Ums.Application.Test/Tenants/Branding`, a nivel de handler y sin -/// estado compartido, que es donde corresponde mientras esta suite no aísle. +/// La tercera prueba —leer lo que el POST acaba de escribir— tumbó 133 pruebas en CI la +/// primera vez, porque las 31 clases de esta suite compartían el mismo almacén InMemory y escribir +/// aquí contaminaba a todas. Con el aislamiento por instancia de factoría (TD-011) vuelve a ser +/// segura, y se conserva precisamente como centinela: si alguien reintroduce el almacén +/// compartido, esta prueba lo delata. /// public sealed class TenantBrandingRestQueryTests : IClassFixture { @@ -67,4 +66,40 @@ public async Task GetBranding_InquilinoInexistente_Responde404() response.StatusCode.Should().Be(HttpStatusCode.NotFound); } + + [Fact] + public async Task GetBranding_DevuelveLoQueElPostAcabaDeEscribir() + { + var tenantId = await PrimerInquilinoSembradoAsync(); + + var cuerpo = new + { + logo = "https://cdn.example.com/logo.png", + // Nombres EXACTOS del enum de dominio (LogoFormat, BackgroundStyle): el handler los + // resuelve con DomainEnumerationParser.FromName, que distingue mayúsculas. 'svg' o + // 'solid' devuelven 400. + logoFormat = "Svg", + primaryColor = "#123456", + backgroundStyle = "SolidColor", + headlineText = "Portal de pruebas", + secondaryText = "Subtítulo de pruebas", + primaryButtonLabel = "Entrar", + footerText = "Pie de pruebas", + customDomain = (string?)null, + magicLinkFallbackEnabled = false, + }; + + var escritura = await _client.PostAsJsonAsync( + $"/api/v1/tenants/{tenantId}/branding", cuerpo, TestContext.Current.CancellationToken); + escritura.StatusCode.Should().Be(HttpStatusCode.Created); + + var lectura = await _client.GetAsync( + $"/api/v1/tenants/{tenantId}/branding", TestContext.Current.CancellationToken); + + lectura.StatusCode.Should().Be(HttpStatusCode.OK); + using var payload = JsonDocument.Parse( + await lectura.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + payload.RootElement.GetProperty("primaryColor").GetString().Should().Be("#123456"); + payload.RootElement.GetProperty("headlineText").GetString().Should().Be("Portal de pruebas"); + } } diff --git a/src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs b/src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs index bed3b63d..dc62c002 100644 --- a/src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs +++ b/src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs @@ -23,6 +23,22 @@ public sealed class UmsApiWebApplicationFactory : WebApplicationFactory // InMemory`) que se filtraban a los hosts de PostgreSqlWebApplicationFactory y // dejaban su UmsPlatformDbContext sin registrar — la causa raíz de ~80 fallos // de integración (G-014). + + /// + /// Sufijo único por INSTANCIA de factoría (TD-011). + /// + /// El proveedor InMemory de EF Core indexa cada almacén por su NOMBRE dentro del proceso. + /// Con los literales fijos que había antes —«TestDb», «TestProjectionDb», «TestReadModelDb»— + /// las 31 clases de esta suite, cada una con su propia factoría, compartían exactamente los + /// mismos tres almacenes. Escribir en una clase se veía desde todas las demás. + /// + /// Lo que costó averiguarlo: una sola prueba que escribía identidad visual sobre el + /// inquilino sembrado tumbó 133 de 316 en CI, y pasaba en local en Debug y en Release porque + /// el orden de ejecución la escondía. Con el nombre por instancia, cada clase arranca de la + /// misma semilla y no puede contaminar a ninguna otra. + /// + private readonly string _almacenAislado = Guid.NewGuid().ToString("N"); + protected override void ConfigureWebHost(IWebHostBuilder builder) { builder.UseEnvironment("Development"); @@ -75,8 +91,8 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) // —opciones genéricas, la configuración de opciones específica del proveedor (EF Core 9+) y // el propio contexto— y se re-registran AMBOS sobre InMemory, de modo que NO quede ningún // servicio del proveedor Npgsql en el contenedor cuando el host es InMemory. - ReplaceDbContextWithInMemory(services, "TestDb"); - ReplaceDbContextWithInMemory(services, "TestProjectionDb"); + ReplaceDbContextWithInMemory(services, $"TestDb-{_almacenAislado}"); + ReplaceDbContextWithInMemory(services, $"TestProjectionDb-{_almacenAislado}"); // G-014 (causa raíz del CUELGUE en CI): el `ReadModelDbContext` (proyecciones de fase 1) // seguía registrado con Npgsql apuntando a `127.0.0.1:5433` (appsettings.Development). En // el host InMemory, cualquier proyección (p. ej. `PermissionTemplatePublishedEvent`) hacía @@ -84,7 +100,7 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) // fallo se enmascaraba, pero en un entorno limpio (runner de CI, contenedor) da «Connection // refused» y, con `EnableRetryOnFailure(3)`, se cuelga en reintentos. Se re-registra también // sobre InMemory para que el host InMemory NO toque PostgreSQL en absoluto. - ReplaceDbContextWithInMemory(services, "TestReadModelDb"); + ReplaceDbContextWithInMemory(services, $"TestReadModelDb-{_almacenAislado}"); services.RemoveAll(); services.RemoveAll();