Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 0
Assert the worker's registrations, and describe the pass it actually has#26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -8,11 +8,25 @@ Applies to `src/MyApp.Worker/`. | ||
| - **`BackgroundService` + `PeriodicTimer`.** No scheduling framework by default — most workers are "every N minutes" or "drain this," and the BCL does both with no dependencies. | ||
| - **Escalate to Quartz.NET only for a stated need:** cron expressions, clustering, or persistent job state. Adding it speculatively buys a database table and a configuration surface you do not want. | ||
| - **Hangfire is out.** `Hangfire.Core` is LGPL v3, which is a licence to adopt deliberately rather than inherit, and its Pro tier is paid. Its dashboard was compensating for missing observability, which Aspire already provides. | ||
| - **`ExecuteAsync` contains no business logic.** It ticks, runs one pass, and refuses to let a failure end the loop. Resolving a scope and dispatching to a handler happens a level down in `RunOnceAsync` — that is the position an endpoint occupies in the API archetype, and keeping the two apart is what makes either testable without the other. | ||
| - **`ExecuteAsync` contains no business logic.** It ticks, runs one pass, and refuses to let a failure end the loop. Resolving a scope and dispatching to a handler belongs to a **pass** of its own — that is the position an endpoint occupies in the API archetype, and giving it a separate type is what lets either be tested without the other. | ||
| ```csharp | ||
| // One pass: a scope, a dispatch, and what the outcome means. Everything here is decided by the | ||
| // result, so it can be exercised with no clock and no timer. | ||
| internal sealed class ReconcileOrdersPass(IServiceScopeFactory scopes) | ||
| { | ||
| public async Task RunAsync(CancellationToken ct) | ||
| { | ||
| await using var scope = scopes.CreateAsyncScope(); | ||
| var handler = scope.ServiceProvider.GetRequiredService<IHandler<Request, Response>>(); | ||
| var result = await handler.HandleAsync(new Request(), ct); | ||
| // map result category to retry / dead-letter / log — never throw to signal it | ||
| } | ||
| } | ||
| // The schedule, and one guarantee: a failed pass must not end the loop. | ||
| internal sealed partial class ReconcileOrdersWorker( | ||
| IServiceScopeFactory scopes, | ||
| ReconcileOrdersPass pass, | ||
| TimeProvider clock, | ||
| ILogger<ReconcileOrdersWorker> logger) | ||
| : BackgroundService | ||
| @@ -32,7 +46,7 @@ internal sealed partial class ReconcileOrdersWorker( | ||
| { | ||
| try | ||
| { | ||
| await RunOnceAsync(ct); | ||
| await pass.RunAsync(ct); | ||
| } | ||
| catch (Exception exception) | ||
| when (exception is not OperationCanceledException || !ct.IsCancellationRequested) | ||
| @@ -42,15 +56,6 @@ internal sealed partial class ReconcileOrdersWorker( | ||
| } | ||
| } | ||
| // ExecuteAsync schedules and keeps the loop alive; one pass lives here. | ||
| internal async Task RunOnceAsync(CancellationToken ct) | ||
| { | ||
| await using var scope = scopes.CreateAsyncScope(); | ||
| var handler = scope.ServiceProvider.GetRequiredService<IHandler<Request, Response>>(); | ||
| var result = await handler.HandleAsync(new Request(), ct); | ||
| // map result category to retry / dead-letter / log — never throw to signal it | ||
| } | ||
| [LoggerMessage(Level = LogLevel.Error, Message = "A pass threw and was swallowed to keep the worker alive.")] | ||
| private static partial void Failed(ILogger logger, Exception exception); | ||
| } | ||
| @@ -69,5 +74,5 @@ internal sealed partial class ReconcileOrdersWorker( | ||
| ### Testing | ||
| - **Handlers are tested directly**, against Testcontainers Postgres, with Respawn between tests. There is no transport to go through, so the handler *is* the entry point. | ||
| - **Separate the pass from the schedule, and test the pass.** `RunOnceAsync` holds everything decided by a result — the scope, the dispatch, retry against dead-letter — so it can be exercised against a stub handler with no clock and no timer. Make the retry backoff a setting so a test can set it to zero. | ||
| - **Separate the pass from the schedule, and test the pass.** The pass holds everything decided by a result — the scope, the dispatch, retry against dead-letter — so it can be exercised against a stub handler with no clock and no timer. Make the retry backoff a setting so a test can set it to zero. | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| - **Test one guarded iteration, not the timer.** Extract the `try`/`catch` into an `internal` method — with `<InternalsVisibleTo Include="MyApp.Worker.Tests" />` on the host project — so a test can call it directly and assert which exceptions survive it. That a failing pass does not end the loop is your logic; that `PeriodicTimer` fires is the BCL's. Driving a real `BackgroundService` from a fake clock needs the scheduler to hand off between advancing the clock and the loop resuming — a test that does it is flaky unless it sleeps, and then it is a slow test of someone else's code. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| using FluentValidation; | ||
| using Loom.Handlers; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| namespace MyApp.Worker.Tests; | ||
| /// <summary> | ||
| /// Every handler and every validator resolves. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// A validator that is not registered is silently ignored — the decorator treats a missing one as | ||
| /// nothing to validate, which is right for a slice with no rules and disastrous for one with them. | ||
| /// Assembly scanning excludes internal types unless asked, and slice validators are internal. | ||
| /// <para> | ||
| /// Both sets are discovered rather than listed. Naming one handler would only ever prove that handler | ||
| /// is registered, so a new slice whose <c>AddHandler</c> call was forgotten would still pass — and in a | ||
| /// worker the first sign of that is a tick failing in a deployment, with no request to fail instead. | ||
| /// </para> | ||
| /// </remarks> | ||
| [NotInParallel] | ||
| public sealed class RegistrationTests | ||
| { | ||
| [Test] | ||
| public async Task Every_Validator_In_The_Application_Resolves() | ||
| { | ||
| using IServiceScope scope = WorkerFixture.Host.CreateScope(); | ||
| Type[] requestTypes = | ||
| [ | ||
| .. typeof(WorkerServices).Assembly.GetTypes() | ||
| .Where(type => type is { IsClass: true, IsAbstract: false } | ||
| && type.BaseType is { IsGenericType: true } baseType | ||
| && baseType.GetGenericTypeDefinition() == typeof(AbstractValidator<>)) | ||
| .Select(type => type.BaseType!.GetGenericArguments()[0]), | ||
| ]; | ||
| await Assert.That(requestTypes).IsNotEmpty(); | ||
| foreach (Type requestType in requestTypes) | ||
| { | ||
| Type validatorType = typeof(IValidator<>).MakeGenericType(requestType); | ||
| await Assert.That(scope.ServiceProvider.GetService(validatorType)).IsNotNull(); | ||
Comment on lines
+30
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bashset -e
printf'%s\n''--- RegistrationTests.cs outline ---'
ast-grep outline src/Loom.Templates/templates/loom-worker/tests/MyApp.Worker.Tests/RegistrationTests.cs ||trueprintf'%s\n''--- RegistrationTests.cs ---'
cat -n src/Loom.Templates/templates/loom-worker/tests/MyApp.Worker.Tests/RegistrationTests.cs
printf'%s\n''--- WorkerServices.cs ---'
cat -n src/Loom.Templates/templates/loom-worker/src/MyApp.Worker/WorkerServices.cs
printf'%s\n''--- validator declarations and inheritance ---'
rg -n --glob '*.cs''class .*Validator|AbstractValidator|IValidator<' src/Loom.Templates/templates/loom-worker
printf'%s\n''--- project and test files ---'
git ls-files src/Loom.Templates/templates/loom-worker/tests/MyApp.Worker.Tests src/Loom.Templates/templates/loom-worker/src/MyApp.Worker | sed -n '1,200p'Repository: CodeByDylan/Loom Length of output: 9133 🏁 Script executed: #!/bin/bashset -e
printf'%s\n''--- validator and registration conventions ---'
rg -n -i --glob '!**/bin/**' --glob '!**/obj/**' \
'AbstractValidator|AddValidatorsFromAssembly|validator|direct inheritance|base class' \
src/Loom.Templates README.md .github 2>/dev/null | sed -n '1,240p'printf'%s\n''--- relevant project files ---'
cat -n src/Loom.Templates/templates/loom-worker/tests/MyApp.Worker.Tests/MyApp.Worker.Tests.csproj
cat -n src/Loom.Templates/templates/loom-worker/src/MyApp.Worker/MyApp.Worker.csproj
printf'%s\n''--- validator source ---'
cat -n src/Loom.Templates/templates/loom-worker/src/MyApp.Worker/Features/Widgets/RetireOversizedWidgets.cs
printf'%s\n''--- test fixture and registration usages ---'
rg -n -A8 -B8 --glob '*.cs' \
'class WorkerFixture|AddWorkerServices|Every_Validator_In_The_Application_Resolves|GetServices|GetService' \
src/Loom.Templates/templates/loom-worker/tests/MyApp.Worker.Tests \
src/Loom.Templates/templates/loom-worker/src/MyApp.Worker
printf'%s\n''--- read-only source verifier ---'
python3 - <<'PY'from pathlib import Pathimport reroot = Path("src/Loom.Templates/templates/loom-worker")classes = []for path in root.rglob("*.cs"): text = path.read_text() for m in re.finditer( r"\b(?:internal|public|private|protected)?\s*(?:sealed\s+|abstract\s+)?" r"class\s+(\w+)\s*:\s*([^{\n]+)", text, ): bases = [x.strip() for x in m.group(2).split(",")] if any("AbstractValidator" in base or "IValidator<" in base for base in bases): classes.append((str(path), m.group(1), bases))print("validator-like declarations:")for item in classes: print(item)print("direct AbstractValidator<T> declarations:", sum(any(re.fullmatch(r"AbstractValidator<[^>]+>", b) for b in bases) for _, _, bases in classes))print("validator-like declarations found:", len(classes))PYRepository: CodeByDylan/Loom Length of output: 27161 🌐 Web query:
💡 Result: When using FluentValidation.DependencyInjectionExtensions, the AddValidatorsFromAssembly (and related methods like AddValidatorsFromAssemblyContaining) automatically registers validators by scanning an assembly for classes that implement IValidator [1][2]. Regarding inheritance and multiple validators, here is how the system behaves: Registration Behavior The AddValidatorsFromAssembly method registers each discovered validator as both the interface IValidator and as its own concrete type [3]. It does not automatically process inheritance hierarchies [4]. For example, if you have a Base class and a Derived class, a validator for the Base class will not be automatically used when the system attempts to validate the Derived class because the DI container specifically looks for IValidator [4][5]. Inheritance and Validation If you need to validate a base class or an object that can be one of several subclasses, the official recommended approach is to use SetInheritanceValidator within the parent validator [6][7]. This method requires you to explicitly map every concrete subclass you intend to support [6][5]. The library intentionally avoids automatic inheritance-based resolution to prevent non-obvious or conflicting behaviors [5]. Multiple Validators If you have multiple validators registered for the same model type, ASP.NET Core's service resolution typically picks the first one registered, making the order unpredictable [8]. If you need specific control over which validator is used, you should avoid automatic assembly scanning for those specific types and manually register your preferred validator with the DI container [9][8]. Summary of Recommendations 1. For simple, one-to-one validator-to-model mappings: Use AddValidatorsFromAssembly [1][10]. 2. For inheritance scenarios: Use SetInheritanceValidator and explicitly define child mappings in your parent validator [6][7]. 3. For fine-grained control: Manually register your validator mappings (e.g., services.AddScoped<IValidator, TValidator>) instead of relying on automatic assembly scanning [8][10]. Citations:
Discover validators through
🤖 Prompt for AI Agents | ||
| } | ||
| } | ||
| [Test] | ||
| public async Task Every_Handler_Resolves_Through_Its_Decorator_Chain() | ||
| { | ||
| using IServiceScope scope = WorkerFixture.Host.CreateScope(); | ||
| Type[] handlerInterfaces = | ||
| [ | ||
| .. typeof(WorkerServices).Assembly.GetTypes() | ||
| .Where(type => type is { IsClass: true, IsAbstract: false }) | ||
| .SelectMany(type => type.GetInterfaces()) | ||
| .Where(contract => contract.IsGenericType | ||
| && contract.GetGenericTypeDefinition() == typeof(IHandler<,>)) | ||
| .Distinct(), | ||
| ]; | ||
| await Assert.That(handlerInterfaces).IsNotEmpty(); | ||
| foreach (Type contract in handlerInterfaces) | ||
| { | ||
| await Assert.That(scope.ServiceProvider.GetService(contract)).IsNotNull(); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.