test: rewrite weak tests for real behavior coverage#197
Draft
antosubash wants to merge 5 commits into
Draft
Conversation
Pure-noise tests removed: - TenantIdTests, TenantHostIdTests — value-object equality is guaranteed by the C# record struct compiler. - ExceptionTests — 16 tests of constructor passthrough and string interpolation; no behavior under test. - IModuleTests.DefaultMethods_DoNotThrow — interface default-method invocation cannot fail. - RateLimitingModuleTests — asserts hardcoded compile-time policy config; refactor-and-rename tests with no domain logic. - DevTools/FileWatcherIntegrationTests — three smoke tests that start a file watcher and assert no exception. Never verified the watcher actually noticed the file change. - OpenIddict ConnectEndpointTests — tests that OpenIddict middleware rejects bad requests; framework behavior, not module logic. The AreRegistered test was an explicit not-404 smoke check. - Admin GetUsersEdit_ExistingUser_Returns200 — was Skipped due to test infrastructure gap; the live _NonExistent_Returns404 sibling already covers the routing. Extracted FakeHostEnvironment to its own file (was used by ServiceLifecycleTests too). ~30 tests deleted, no coverage lost.
Dashboard previously had zero tests despite shipping two view endpoints and a POST tick endpoint. HomeEndpoint: - Anonymous request renders with isAuthenticated=false, displayName="User". - Authenticated request renders with displayName from ClaimTypes.Name. BroadcastingEndpoint: - GET /broadcasting authenticated renders the channel name derived from the user's NameIdentifier claim and exposes the tick fire URL. - POST /api/dashboard/broadcasting/tick rejects anonymous requests with 401 and returns 204 NoContent when authenticated. 5 new behavioral tests, all assert on Inertia component name and props (not just status codes).
The old UserServiceTests mocked UserManager<ApplicationUser> end-to-end via NSubstitute, so the tests proved that calling Mock.Foo returns the preset value — not that the user actually persists, that the password is hashed, or that uniqueness is enforced. The duplicate-email scenario in particular only verified the mock returned an IdentityResult.Failed, not that EF Core's Identity stack would reject it. Rewrote UserServiceTests as integration tests that resolve the real UserManager / RoleManager from SimpleModuleWebApplicationFactory's in-memory SQLite host: - CreateUserAsync persists the row, hashes the password (PasswordHash != plaintext, CheckPasswordAsync accepts it), publishes UserCreated. - Duplicate-email creation throws ValidationException and does NOT publish a second UserCreated event. - Weak password (rejected by Identity's PasswordValidator) throws ValidationException. This scenario was untestable with a mocked UserManager. - UpdateUserAsync re-queries the DB to confirm the change persisted rather than trusting the return value. - DeleteUserAsync verifies the row is gone via FindByIdAsync. - GetRoleIdsByNamesAsync verifies real RoleManager-backed lookup. Also trimmed UserIdTests to the two genuine custom-validator scenarios (empty/whitespace rejection) — dropped the From-with-valid-string, equality, and ToString tests since those exercise Vogen plumbing the type system already guarantees. Removed the now-unused NSubstitute package reference. 11 new behavioral tests, ~9 trivial mock tests deleted, net +2.
…odes The Admin endpoint tests previously only checked that POST/PUT/DELETE returned a redirect. They never re-queried the DB to verify the change took effect, so any regression that broke the underlying service call while still returning a 302 (e.g. silently swallowing an exception) would slip through. User endpoint rewrites: - UpdateUser now fetches the user post-redirect and asserts DisplayName and Email actually changed. - LockUser asserts LockoutEnd is set to a future timestamp. - UnlockUser locks first, then unlocks, and asserts LockoutEnd is null or in the past. - DeactivateUser asserts DeactivatedAt is set near UtcNow. - ReactivateUser deactivates first, then reactivates and asserts DeactivatedAt is cleared. - ResetPassword captures the old PasswordHash, performs the reset, then asserts the new password authenticates and the old one no longer does — catches a bug class (silent no-op) that the redirect check missed. Role endpoint rewrites: - CreateRole looks up the role by name post-redirect. - UpdateRole asserts the new name + description persisted. - DeleteRole asserts the role no longer exists in the DB. Smoke tests for GET endpoints (Returns200, Unauthenticated_Returns401) left in place — they're the lightweight routing-and-auth guard, not the behavior test. Same total test count, real behavior coverage.
…r checks BackgroundJobs: - Deleted three Cancel/Retry/DeleteRecurring _DoesNotReturn401 tests. They were placeholders that posted random GUIDs and only asserted "not 401" — tautological with the existing permission-denied tests on the same routes (a view-only client getting 403 already proves manage permission isn't getting 401/403). - Added Retry_WithViewOnlyPermission_Returns403 and DeleteRecurring_WithViewOnlyPermission_Returns403 to round out the permission matrix (Cancel already had its 403 test). Settings: - UpdateSetting test now PUTs, then GETs, and asserts the returned value matches what was written (was previously a status-only test that ignored the response body entirely). - DeleteSetting test now creates, deletes, then GETs and asserts 404, proving the row was actually removed. - GetDefinitions test asserts the response is a non-empty JSON array whose items expose a "key" property — proves the registry is wired to the endpoint, not just that the route returns 200. - UpdateMySetting now reads /api/settings/me after the update and asserts the user-scoped value appears in the resolved list. All 25 Settings tests + 14 BackgroundJobs endpoint tests pass.
Deploying simplemodule-website with
|
| Latest commit: |
e6573d3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://27d0504a.simplemodule-website.pages.dev |
| Branch Preview URL: | https://claude-review-test-suite-3s2.simplemodule-website.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR cleans up and strengthens the test suite by removing low-signal tests and replacing status-code/mock-only assertions with integration tests that verify observable behavior (DB state, Identity password hashing, published events, and JSON response bodies).
Changes:
- Deleted a set of “noise” tests (value-object equality plumbing, exception-message formatting, framework behavior, and no-op smoke tests).
- Added/rewrote integration tests across Dashboard, Users, Admin, Settings, and BackgroundJobs to assert real outcomes (persistence, permissions, returned props/bodies, and events).
- Removed unused mocking infrastructure (e.g., NSubstitute) and refactored shared test helpers (e.g.,
FakeHostEnvironment).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SimpleModule.DevTools.Tests/FileWatcherIntegrationTests.cs | Removed low-signal file watcher smoke/integration tests. |
| tests/SimpleModule.DevTools.Tests/FakeHostEnvironment.cs | Extracted FakeHostEnvironment into a reusable helper for DevTools tests. |
| tests/SimpleModule.Core.Tests/IModuleTests.cs | Removed default-interface-method no-op test; kept meaningful service registration test. |
| tests/SimpleModule.Core.Tests/ExceptionTests.cs | Removed exception message/constructor formatting tests. |
| modules/Users/tests/SimpleModule.Users.Tests/Unit/UserServiceTests.cs | Removed mocked UserManager unit tests that only validated mock wiring. |
| modules/Users/tests/SimpleModule.Users.Tests/Unit/UserIdTests.cs | Trimmed UserId tests to custom validation scenarios; removed VO equality/plumbing assertions. |
| modules/Users/tests/SimpleModule.Users.Tests/SimpleModule.Users.Tests.csproj | Dropped unused NSubstitute package reference. |
| modules/Users/tests/SimpleModule.Users.Tests/Integration/UserServiceTests.cs | Added integration tests using real Identity/EF to verify hashing, persistence, events, and role lookup. |
| modules/Tenants/tests/SimpleModule.Tenants.Tests/Unit/TenantIdTests.cs | Removed value-object equality/plumbing tests. |
| modules/Tenants/tests/SimpleModule.Tenants.Tests/Unit/TenantHostIdTests.cs | Removed value-object equality/plumbing tests. |
| modules/Settings/tests/SimpleModule.Settings.Tests/Integration/SettingsEndpointTests.cs | Reworked settings endpoint tests to validate stored/returned values and definitions payload shape. |
| modules/RateLimiting/tests/SimpleModule.RateLimiting.Tests/RateLimitingModuleTests.cs | Removed policy registry assertion tests tied to compile-time config details. |
| modules/OpenIddict/tests/SimpleModule.OpenIddict.Tests/Integration/ConnectEndpointTests.cs | Removed framework-behavior tests that asserted OpenIddict middleware outcomes. |
| modules/Dashboard/tests/SimpleModule.Dashboard.Tests/Integration/HomeEndpointTests.cs | Added integration tests asserting Inertia component + props for Home endpoint. |
| modules/Dashboard/tests/SimpleModule.Dashboard.Tests/Integration/BroadcastingEndpointTests.cs | Added integration tests asserting Inertia props and 401/204 behavior for broadcasting tick. |
| modules/BackgroundJobs/tests/SimpleModule.BackgroundJobs.Tests/Integration/BackgroundJobsEndpointTests.cs | Replaced tautological “not 401” tests with concrete permission-matrix assertions (403 for view-only). |
| modules/Admin/tests/SimpleModule.Admin.Tests/Integration/AdminUsersEndpointTests.cs | Updated admin user mutation tests to re-query DB and assert persisted mutations (lock, deactivate, reset password, etc.). |
| modules/Admin/tests/SimpleModule.Admin.Tests/Integration/AdminRolesEndpointTests.cs | Updated role mutation tests to assert persistence via RoleManager re-queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+31
to
+34
| updateResponse.StatusCode.Should().Be(HttpStatusCode.NoContent); | ||
|
|
||
| var getResponse = await client.GetAsync($"/api/settings/{key}?scope=1"); | ||
| getResponse.StatusCode.Should().Be(HttpStatusCode.OK); |
| && def.TryGetProperty("key", out var key) | ||
| && key.GetString() == "app.theme" | ||
| ); | ||
| themeEntry.ValueKind.Should().NotBe(JsonValueKind.Undefined); |
| { | ||
| _factory = factory; | ||
| // Force the host to spin up so the in-memory SQLite schema exists. | ||
| _ = _factory.CreateClient(); |
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
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.
Summary
Cleanup pass on the test suite informed by a quality review across all 20 test projects (439 tests, ~9.6k LOC). Tests that asserted nothing meaningful — value-object equality on generated record structs, exception-message string interpolation, status codes without behavior checks,
UserManagercalls against a mockedUserManager, etc. — have been deleted or rewritten to assert observable behavior (DB state, hashed password values, published events, response bodies).Commits
test: delete tests that don't test anything— drops ~30 noise tests: VO equality (TenantIdTests,TenantHostIdTests), exception string formatting (ExceptionTests), interface-default no-op (IModuleTests.DefaultMethods_DoNotThrow), hardcoded compile-time config (RateLimitingModuleTests), DevTools "no exception thrown" smoke tests (FileWatcherIntegrationTests), and OpenIddict framework-behavior tests (ConnectEndpointTests). The one[Skip]'d Admin test that the live_NonExistent_Returns404sibling already covered is also gone.test(dashboard): cover Home and Broadcasting endpoints— Dashboard module had zero tests despite shippingHomeEndpointandBroadcastingEndpoint. Added 5 integration tests that assert on Inertia component name + props (not just status codes): anonymous/authenticateddisplayNameresolution on Home, channel derivation fromNameIdentifieron Broadcasting, and 401/204 contract on the tick POST.test(users): replace mocked UserManager with real Identity + EF—UserServiceTestspreviously mockedUserManager<ApplicationUser>end-to-end via NSubstitute, so the tests only proved that the mock returned its preset values. Rewrote as integration tests using the factory's real Identity stack:PasswordHash != plaintext,CheckPasswordAsyncaccepts the original).IdentityResult.Failed), and noUserCreatedEventis published on the rejected attempt.GetRoleIdsByNamesAsyncuses realRoleManagerinstead of pre-canned dictionaries.Also trimmed
UserIdTeststo the two genuine custom-validator scenarios (Vogen plumbing tests deleted) and dropped the unused NSubstitute package reference.test(admin): assert DB state after mutations instead of just status codes— every Admin user/role mutation test previously stopped at the redirect status. Any regression that swallowed the underlying service call while still returning 302 (e.g., silent exception, no-op stub) would pass. Each mutation test now re-queries the DB:UpdateUserconfirmsDisplayNameandEmailchanged.LockUserassertsLockoutEndis in the future;UnlockUserfirst locks then unlocks and asserts the lockout is cleared.DeactivateUser/ReactivateUserassertDeactivatedAtis set / cleared.ResetPasswordcaptures the oldPasswordHash, performs the reset, then asserts the new password authenticates and the old one no longer does — catches the silent-no-op class of bug the redirect check missed entirely.test(jobs,settings): replace status-code-only assertions with behavior checks_DoesNotReturn401placeholder tests that posted random GUIDs and only asserted "not 401" — tautological with the existing 403 permission-denied tests on the same routes. AddedRetryandDeleteRecurringview-only-permission-returns-403 tests to round out the permission matrix.UpdateSettingwrites then reads back and asserts the value matches;DeleteSettingconfirms a subsequent GET returns 404;GetDefinitionsasserts the response is a JSON array of items with akeyproperty;UpdateMySettingreads/api/settings/meand confirms the user-scoped value appears in the resolved list.Scope notes
Two of the buckets in the original review didn't deliver further changes here:
HostDbContext,SimpleModuleOptions,SimpleModule.Hosting.RateLimiting), so compiling generated output in isolation requires stubbing the full host surface. The implicit safety net already exists: every module integration test runs through the real generator output viaSimpleModuleWebApplicationFactory. Adding more isolated compile-and-execute tests would duplicate that coverage at high complexity cost. Left the 195 brittle string-match tests alone — they at least catch breakage on individual aspects (SM00xx diagnostics, generated tree filenames).Net effect
SimpleModule.Cli.Testsis intentionally excluded fromdotnet testviaIsTestProject=false— scaffolding tests spawndotnet buildand run ~15min).Test plan
dotnet testpasses for every in-scope project (Admin 28/28, Settings 25/25, BackgroundJobs 96/96, Users 54/54, Dashboard 5/5 new, plus all unchanged modules).Generated by Claude Code