test: cobertura y verificación de la solución del Sprint 1 - #2
Merged
Merged
Conversation
The project had no coverage measurement, so there was no way to tell which branches the suite actually exercises. The agent instruments the unit test run and the report lands in target/site/jacoco. Records, enums, the Spring entry point, the @ConfigurationProperties holders and the DTOs are excluded: they carry no branches of their own, so counting them raises the percentage without saying anything about the behaviour under test. Baseline on this commit: 20.1% of instructions, 25.0% of branches.
…ases The eight use cases of the two modules had no unit tests at all: the only thing exercising them was the integration flow, which walks the happy path. What was missing is the behaviour at the edges, and that is where the rules live. Techniques, not line filling. Equivalence partitioning and boundary values on the lock threshold (max-1, max, max+1) and on the age rule (the day before, the day of and the day after the eighteenth birthday, plus a leap-day birth). A decision table over every value of UpstreamAuthError, to pin down which failures count towards the lock and which do not. The full 3x3 state transition table of Client, with the four cells that must reject and the two that are idempotent. Interactions are asserted too, because several rules are about what must NOT happen: a locked account never reaches the identity provider, a weak password never reaches it either, and an unknown email in the resend flow neither calls out nor throws, which is what keeps user enumeration shut. The compensation path is now pinned: if saving the client fails, the auth user is deleted and the original exception travels on. 269 new cases. The nine classes involved go to 100% of instructions and branches. Whole suite: 296 unit tests plus 21 integration, green. Coverage 71.5% -> 76.1% of instructions, 45.5% -> 60.3% of branches.
GoTrueClient translates the identity provider's answers into the domain's error vocabulary, and that translation had 39 of its 45 branches untested. It is the class where a silent mistake costs the most: every wrong mapping becomes a wrong HTTP status for the client. mapError is a decision table, so it is tested as one: 30 rows over (context, HTTP status, response body), plus 6 rows that exist only to pin the order in which the rules fire. Each of the eight endpoints gets its happy path, its mapped error and a network failure. The headers are asserted too, because the rule that the secret key only travels to /admin/** is a security rule, not a detail. The adapters are tested against a mocked repository: what matters there is the translation and the lowercasing of the email before it reaches the query, not the database, which the integration tests already cover. 112 new cases. GoTrueClient goes from 19% to 100% of instructions and from 13% to 100% of branches; the adapters and the mapper from 0% to 100%. Whole suite: 408 unit tests plus 21 integration, green. Coverage 76.1% -> 94.4% of instructions, 60.3% -> 86.5% of branches.
…r edges GlobalExceptionHandler decides the status, the body and the error code of every failed request, and not one of its sixteen branches was covered. It is now tested by calling it directly, one nested group per handler, asserting status, errorCode, type, instance and traceId on each. Two of those tests exist to prove a negative. handleUnexpected is given an exception whose message carries a JDBC connection string with a password in it, and the response is checked to contain neither the message, nor the exception class, nor a stack trace. A security property nobody had written down is now pinned, and it holds. The two domain policies get their boundaries. The password length at 7, 8, 71, 72 and 73; each character class violated on its own and all four at once; a check that isValid always agrees with violations().isEmpty(). The lock window at its exact edges: an attempt landing on now - lockWindow counts, one millisecond earlier does not, and an attempt dated after now is discarded. BusinessException is checked for what it promises rather than what it stores: that details() rejects mutation and that it copies the incoming map, so a caller cannot reach in and change an exception after throwing it. 336 new cases. The ten classes involved go to 100% of instructions and branches. Whole suite: 744 unit tests plus 21 integration, green. Coverage 94.4% -> 98.4% of instructions, 86.5% -> 94.2% of branches.
The two controllers, the JWT role converter, the trace filter and the security configuration were all at zero. The converter is the only authorisation barrier in the system and no test executed it: the integration tests inject the authorities by hand and skip it entirely. Two security properties are now pinned by tests that prove a negative. A role placed in user_metadata, which the client can edit, grants nothing: only app_metadata is read, and when both carry a role app_metadata wins. That is checked as a unit and again over HTTP through the real filter chain. And the trace filter clears the MDC in its finally block even when the chain throws, so no request inherits another request's correlation id. Every endpoint's declared status code is locked down, because a change there breaks consumers silently, and the whole ErrorCode to HTTP status table is asserted through the controllers. The DTO boundaries are checked against the validator directly: each field at its exact limit and one past it. A 90% floor on instructions and branches now fails the build. It was verified in both directions: it passes with the full suite and it does fail when coverage drops. The DTO and the properties records are no longer excluded from the measurement, because excluding them raises the number without saying anything about what is tested. docs/qa/informe-pruebas-sprint1.md records the technique behind each group of tests, the defects found and left unfixed, the acceptance criteria still unverified, and eight design cards for the team. 302 new cases. Whole suite: 1046 unit tests plus 21 integration, green. Coverage over the full scope: 100% of instructions, branches, lines and methods (2976, 156, 678 and 163 respectively).
The suite had grown to 1046 cases for 156 branches and 2102 lines of production code. That ratio does not hold up, and the excess was not spread evenly: it sat almost entirely on enums and records. ErrorCodeTest carried 85 cases for an enum of sixteen values, most of them asserting that valueOf round-trips and that name() is not null, which tests the JVM. It keeps the sixteen-row decision table that pins each code to its HTTP status, because that one is a contract consumers depend on: change DUPLICATE_EMAIL from 409 and a client breaks. The tests for AppRole, AuthTokens, ConfirmedUser, UpstreamAuthError, UpstreamAuthException, ClientStatus, NotificationChannel, RegistrationOutcome and RegisterClientCommand are gone entirely. They asserted over compiler generated equals, hashCode and toString, and over enum constants. The measurement makes the case on its own: 156 cases removed and coverage did not move. Instructions, branches, lines and methods all stay at 100% over the same 52 classes. Those cases were covering nothing that other tests were not already covering, so all they added was execution time and maintenance. 890 unit tests plus 21 integration, green.
Eight surgical changes, each verified against the whole suite before the next
one. No public signature, endpoint path, JSON field name or database schema
was touched.
Password recovery and password reset rethrew the raw UpstreamAuthException,
and nothing handles that type, so the caller got a 500 where the rest of the
module answers 429 or 502. The recovery endpoint promises the same answer for
every email so it cannot be used to find out who is registered, and a provider
rate limit turned that 202 into a 500, which is itself the signal the promise
was meant to hide. The translation now lives in the two use cases, mirroring
LoginUseCase, rather than in a handler in shared: an UpstreamAuthException
handler there would make the shared kernel depend on the auth module and break
the ArchUnit rule that keeps it module agnostic.
GoTrue answers 404 to an expired or already consumed one-time token, and the
404 rule was evaluated before the expired one, so a stale verification link
was reported as USER_NOT_FOUND and the user read "user not found". The expired
check now runs first and the bare 404 is classified by the verify branch. Only
the two rows of the decision table that documented this defect change; the
other thirty-one were verified untouched.
A non-numeric or decimal expires_in, and a malformed id, escaped as raw
NumberFormatException and IllegalArgumentException, outside the two exception
types the adapter catches, and reached the caller as a 500. Both now come out
as UNAVAILABLE, keeping the original as the cause.
Role and error-code normalisation now pass Locale.ROOT. These are protocol
values, not display text: under a Turkish default locale "admin" upper cases
to "ADMİN" and every hasRole("ADMIN") check fails silently.
The two exceptions declare serialVersionUID. BusinessException holds its
details in a LinkedHashMap field rather than a Map, which is what made the
serialisable-field warning legitimate; the published view is still an
unmodifiable copy.
The email of a confirmed user is deliberately left tolerant. Nothing reads it:
ConfirmEmailUseCase resolves the client by id. Requiring it would turn a
response missing that field into a 502 over a value that is discarded, so it
is returned as null and never as the four-character string "null".
Six criteria of HU-001 and HU-021 had no test at all. Three are now verified, two turned out to be defects, and one stays half open on purpose. The one-time link is the property that gives the criterion its name and only expiry was being tested, never reuse. Confirming an email twice with the same token_hash now has to be rejected, and so does redeeming a recovery link twice; a password that fails the policy must not burn the link either. This needed an identity provider double that remembers what it has already issued and consumed, so the existing mock grew that state. The JWT decoder was never executed: the tests injected the authorities directly and skipped it. It now runs against a JWKS served over HTTP from a local server with keys generated in the test, so a wrong issuer, an expired token and a forged signature are rejected for real. No new dependency. Logout does not invalidate the access token. The revocation reaches the provider, but the token is self-contained and validated offline, so it keeps working until it expires. ADR-0003 accepts that trade-off; the criterion is written in absolute terms, so it is recorded as partially met. Spring MVC cannot translate its own status codes. The advice is ordered ahead of everything and handles Exception, so a wrong method answers 500 instead of 405 and without an Allow header, an unsupported media type answers 500 instead of 415, and an unknown path answers 500 instead of 404. Each one is also logged at ERROR with a full stack trace, so ordinary client mistakes read as server failures. The invariant that only a verified client may confirm a booking cannot be verified end to end because there is no booking flow yet. Rather than invent one, it is checked against the aggregate reread from PostgreSQL, and three guards are armed for the day the flow arrives: the invariant must stay public, nobody outside Client may read ClientStatus.ACTIVE, and any future booking class must call canConfirmBooking. The happy path of tomorrow's demo is covered end to end: register, confirm, log in, read the profile, log out, checking the HTTP status and the database row at each step.
# Conflicts: # src/test/java/com/codefactory/bookingplatform/auth/domain/model/UpstreamAuthExceptionTest.java
… cannot Nothing verified that the Spring context loads. A broken bean, a missing property or a schema drift would have been found on deploy, not on build. The cloud profile is now started against a PostgreSQL container with docs/database/schema.sql applied and ddl-auto=validate. Green means the committed DDL and the JPA mapping agree, which is the check that decides whether the service boots on Render at all. Every environment variable the deployment declares is exercised absent, empty and present, with a controlled environment source so the result does not depend on the machine running the tests. Three of them turn out to be read by nobody, and the identity credential turns out not to be required at startup at all: the application comes up without it and fails on the first request, where a login reports INVALID_CREDENTIALS because that is how the provider's 401 is classified. The operator reads "invalid email or password" and looks in the wrong place. That path is pinned end to end. The actuator is pinned as it is actually exposed: which endpoints answer, which are public, and what the health groups contain. The readiness group, which is the one Render polls, does not include the database. The application is also started for real, on Tomcat with a random port and a PostgreSQL container, and answered over HTTP: health, the OpenAPI document, a public endpoint, a protected one without a token, and the trace header. 96 new tests. Nothing in src/main or pom.xml was touched; the fixes those findings call for are written up, not applied.
The report now says which defects were fixed and which were left alone, and why. Five are corrected, one was closed deliberately without requiring the field, and four stay open because closing them means a decision that is not QA's to take, or a risk not worth running the day before a delivery. It also records what was checked outside the test suite: packaging, the Docker image, the compose stack, the cloud profile started against the committed schema, and twenty-four business scenarios exercised over HTTP against the running container. Twenty-two behave as the contract says; the two that do not are the same defect. Six deployment risks are written up with what happens when each environment variable is missing. Three of them matter for a live demo: a missing identity credential lets the application start and makes the login report invalid credentials, which sends whoever is debugging to the wrong place; CORS is declared in render.yaml and read by nobody; and the health check Render polls does not look at the database.
JaCoCo reported 100% of instructions, branches, lines and methods. Mutation
analysis with PIT, running the full mutator set, changed the code 1038 ways
and 82 of those changes went unnoticed. Coverage said the code was exercised;
it was not verified.
The domain and the application layer held: LoginUseCase, RegisterClientUseCase,
Client, the two policies, the JWT role converter, the trace filter, the
adapters and the mapper had no survivors at all. The weakness was in the
wiring and at the HTTP edge.
The token gate was the worst of it. The only assertion on the JWT decoder was
that it is a NimbusJwtDecoder, which passes just as well if the decoder accepts
every algorithm, validates no expiry and trusts any issuer. It is now tested
against a real JWKS endpoint on the loopback with tokens signed in the test:
ES256 and RS256 accepted, HS256 refused, expired refused, foreign issuer
refused.
The whole security filter chain was invisible to the analysis. Spring caches
the test context, so the chain is built once per JVM and only the first test
was credited with covering it; permitAll, anyRequest().authenticated() and
build() could all be removed unnoticed. The chain is now built per test and
39 mutants die against assertions that already existed.
One idiom is worth spreading: jsonPath("$.password").doesNotExist() passes for
a field that is present with a null value, so a test meant to prove the
password never leaves the service could not see the field being sent.
Every surviving mutant was attacked with a test before being called
equivalent; the claimed 111 shrank to 17, each justified individually. The
analysis also exposed four pieces of production code that no test can
distinguish because they do nothing: the CORS customiser with no source bean,
a guard that duplicates what the mapping below it already returns, five
redundant contentType calls and a title that ProblemDetail already derives.
Business scope 91.7% -> 98.3%. Full scope 89.3% -> 95.7%. A 95% threshold is
configured and passing. The run takes two minutes, so it belongs in CI.
An adversarial review broke the production code on purpose and checked which tests stayed green. Four did not protect anything, and one of them was hiding a real defect. Swapping the context literal that GoTrueClient hands its error mapper, so that a recovery failure is classified as a login failure, went unnoticed by all 1046 unit tests and all 21 integration tests. The decision table of thirty seven rows only asserted the resulting error, and its helper matched the request with an empty matcher, so nothing anchored which context each endpoint passes or even which path it calls. Both are now anchored: every row verifies the path, and a new case pins the context of all eight endpoints through the one status that reaches the default arm and names it. PasswordPolicy's isValid was compared against violations().isEmpty(), which is what isValid returns. The assertion was X == X and stayed green with the whole policy disabled. It is replaced by a table with the verdict written down rather than computed. The clock test claimed in its name to detect a frozen clock and did not: a clock frozen at the current instant is still after one minute ago. Reading it twice and waiting for it to move was the first fix and turned out to be flaky, because the system clock resolution does not advance within the loop on Windows. Four clock tests collapse into one that compares against Clock.systemUTC(), which rejects a fixed, an offset and a host-zone clock at once and is deterministic. The timestamp assertion was assertNotNull, which passes for any string. It now parses the value and checks it belongs to this response. Each of the four was verified by reapplying the mutation it is meant to catch and confirming the build turns red, then reverting. The one assertion removed outright was assertNotNull(new JpaAuditingConfig()), which no code can fail. 1018 unit tests plus 98 integration, green. Coverage unchanged at 100%.
The project moved to the organisation repository while this work was in flight, and its main added SonarCloud analysis. Both sides had reached for JaCoCo within a minute of each other, in two different repositories, so the pom conflicted. Both are kept. The four sonar properties come across untouched, including the report path SonarCloud reads. The JaCoCo declaration is this branch's, which is a superset: the same prepare-agent and the same report bound to verify, plus the 90% floor on instructions and branches that fails the build, and the exclusion limited to the bootstrap class. Version 0.8.13 over 0.8.12. Keeping both declarations would have registered the plugin twice. The report the analysis consumes, target/site/jacoco/jacoco.xml, is produced by this configuration, so the coverage raised here reaches SonarCloud without any further change to the workflow. 1018 unit tests plus 98 integration, green, with the floors met.
andersonhg19
changed the title
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
La solución tenía 48 pruebas: 71,5 % de las instrucciones y 45,5 % de las ramas. Esa distancia era el problema — el código se ejecutaba mucho, pero sus decisiones apenas se probaban.
Qué trae
El build ahora falla por debajo del 90 % de cobertura. El umbral está comprobado en los dos sentidos: pasa con la suite completa y rompe el build cuando la cobertura cae.
No es cantidad, es técnica
La cobertura sola no dice nada: se puede recorrer el 100 % de las líneas sin probar una sola decisión. Por eso se aplicaron técnicas formales — valores límite en los umbrales de bloqueo, de longitud de contraseña y de mayoría de edad; una tabla de decisión de 37 filas para la traducción de errores del proveedor; la tabla de transición 3×3 completa del agregado
Client.De hecho la suite llegó a tener 1046 casos unitarios y se recortaron 156 que probaban enums,
recordy comportamiento del propio lenguaje. Al borrarlos la cobertura no se movió del 100 %, lo que demuestra que no cubrían nada que no estuviera ya cubierto.Y se midió si las pruebas de verdad detectan defectos: PIT modifica el código de 1033 formas distintas y la suite caza 1016. Eso destapó cuatro pruebas que no podían fallar —una comparaba
isValidcontra su propia implementación, otra prometía en su nombre detectar un reloj congelado y no lo hacía—, corregidas y demostradas reaplicando la mutación que deben cazar.Cambios en producción
Ocho archivos, +103/−18 líneas. Ninguna firma pública, ruta de endpoint, nombre de campo JSON ni esquema de base de datos cambió.
expires_inno numérico o un identificador malformado escapaban como excepción cruda y acababan en un 500.Locale: bajo locale turcoadminse convierte enADMİNy toda comprobación de rol falla en silencio.Defectos encontrados y no corregidos
Documentados en
docs/qa/informe-pruebas-sprint1.md, con pruebas que fijan el comportamiento actual y fallarán cuando alguien los arregle:GETsobre un endpoint POST devuelve 500 en vez de 405, y sin cabeceraAllow. Lo mismo con 415 y 404. El manejador global se ordena por delante de Spring MVC y tapa sus traducciones. Además cada error corriente de cliente se registra comoERRORcon traza completa.catch.ErrorCode→HttpStatus, contra el ADR-0001, y la regla de ArchUnit no lo detecta porque solo inspecciona dependencias directas.Riesgos de despliegue, con los diffs escritos y sin aplicar
SUPABASE_SECRET_KEYla aplicación arranca igual y el login responde «credenciales inválidas», mandando a quien depure al sitio equivocado.render.yamldeclaraALLOWED_ORIGINSy no la lee ningún código./actuator/healthda 503 pero/actuator/health/readinessda 200, así que Render seguiría enrutando tráfico.No se aplicaron porque cambian el comportamiento de arranque y esa decisión es del equipo.
Verificación
Build limpio desde un clon del repositorio, imagen Docker, stack levantado y 24 escenarios de negocio probados por HTTP contra el contenedor. La suite es estable en tres corridas seguidas, en orden aleatorio de clases y métodos, bajo locale turco, en zona horaria UTC+14 y con la JVM limitada a 1 GB. El perfil
cloudconddl-auto=validatearranca limpio contra elschema.sqldel repositorio, así que el despliegue no muere en el arranque por desajuste de esquema.La configuración de JaCoCo se reconcilió con la de SonarCloud: se conservan las cuatro propiedades
sonar.*y el informe que el análisis consume es exactamente el que esta configuración genera.