diff --git a/docs/docs/how-to/run-conditions.md b/docs/docs/how-to/run-conditions.md index ddc3bf27c5..5a191e476f 100644 --- a/docs/docs/how-to/run-conditions.md +++ b/docs/docs/how-to/run-conditions.md @@ -30,9 +30,13 @@ public class DeployModule : Module - `[RunIfAny]` runs when at least one condition is `true`. Multiple condition attributes are evaluated in this order: `SkipIf`, `RunIfAll`, then -`RunIfAny`. Attribute conditions run during module discovery and register a skipped result -directly. Fluent `.WithSkipWhen(...)` conditions run later in the execution pipeline and invoke -the skipped hooks and lifecycle notifications. +`RunIfAny`. Attribute conditions and fluent `.WithSkipWhen(...)` conditions run in the same +execution pipeline after dependency waiting. Both invoke skipped hooks and lifecycle +notifications. + +Fluent dependencies are validated before execution conditions are evaluated. Every dependency +declared with `DependsOn()` must therefore be registered, even when an attribute condition +will skip the consuming module on the current platform or environment. Built-in platform conditions include `OnLinux`, `OnWindows`, and `OnMacOS`: diff --git a/docs/docs/how-to/skipping.md b/docs/docs/how-to/skipping.md index c9e0c06d6d..fcac16a378 100644 --- a/docs/docs/how-to/skipping.md +++ b/docs/docs/how-to/skipping.md @@ -9,11 +9,9 @@ sidebar_position: 7 The recommended way to configure module skipping is through the `Configure()` method with the fluent builder API: -Attribute conditions (`[SkipIf]`, `[RunIfAll]`, and `[RunIfAny]`) remain supported, -but they run during module discovery, before dependency waiting. An ignored module receives a -skipped result directly. Fluent `.WithSkipWhen(...)` conditions run in the execution pipeline, -where skipped hooks and lifecycle notifications are invoked. Use the fluent API when consumers -depend on those notifications. +Attribute conditions (`[SkipIf]`, `[RunIfAll]`, and `[RunIfAny]`) remain supported. +Attribute and fluent conditions run in the same execution pipeline after dependency waiting, so +both invoke skipped hooks and lifecycle notifications. ### Simple Condition @@ -103,12 +101,11 @@ public class MyModule : Module ## Combining with Other Behaviors -Repeated skip conditions use AND-to-skip semantics. They run in registration order, and the module -is skipped only when every condition returns `SkipDecision.Skip`. A `SkipDecision.DoNotSkip` -result stops evaluation and keeps the module eligible to run. When every condition skips, their -reasons are combined. +Repeated `WithSkipWhen` conditions use OR-to-skip semantics, matching repeated `[SkipIf]` +attributes. They run in registration order, and evaluation stops when any condition returns +`SkipDecision.Skip`. -For example, this module skips cleanup only for CI builds that are not on the main branch: +For example, this module skips cleanup for either CI builds or non-main branches: ```csharp public class CleanupModule : Module @@ -127,8 +124,23 @@ public class CleanupModule : Module } ``` -When any one of several independent predicates should be enough to skip, combine them in a single -condition and return `SkipDecision.Skip` when their OR expression is true. +When every condition must match before the module is skipped, group them explicitly with +`WithSkipWhenAll`: + +```csharp +protected override ModuleConfiguration Configure() => ModuleConfiguration.Create() + .WithSkipWhenAll( + _ => Environment.GetEnvironmentVariable("CI") == "true" + ? SkipDecision.Skip("Running in CI") + : SkipDecision.DoNotSkip, + _ => Environment.GetEnvironmentVariable("DEPLOY_ENV") != "production" + ? SkipDecision.Skip("Not deploying to production") + : SkipDecision.DoNotSkip) + .Build(); +``` + +Conditions inside a `WithSkipWhenAll` group use AND-to-skip semantics and combine their reasons. +The group composes with other skip conditions using OR-to-skip semantics. ## History If a module was skipped, you can attempt to find its history from a previous run. See [History](storing-and-retrieving-results) diff --git a/src/ModularPipelines/Attributes/OperatingSystemConditions.cs b/src/ModularPipelines/Attributes/OperatingSystemConditions.cs index 2b0ca7ed33..1d786511cb 100644 --- a/src/ModularPipelines/Attributes/OperatingSystemConditions.cs +++ b/src/ModularPipelines/Attributes/OperatingSystemConditions.cs @@ -29,43 +29,42 @@ internal static class OperatingSystemConditions /// public static IReadOnlyList GetTargets(IConditionAttribute attribute) { - if (attribute.Logic != ConditionLogic.All) - { - return []; - } + var supportedOperatingSystems = GetSupportedOperatingSystems(attribute); - var conditionTypes = attribute.GetType().GetGenericArguments(); - if (conditionTypes.Length == 0) + if (supportedOperatingSystems is null || supportedOperatingSystems.Count == 0) { return []; } + return [CreateCapability(supportedOperatingSystems)]; + } + + /// + /// Returns whether all-platform attributes require mutually exclusive operating systems. + /// + public static bool HasImpossibleCombination(IEnumerable attributes) + { HashSet? supportedOperatingSystems = null; - foreach (var conditionType in conditionTypes) + foreach (var attribute in attributes) { - var conditionOperatingSystems = GetSupportedOperatingSystems(conditionType); - if (conditionOperatingSystems is null) + var attributeOperatingSystems = GetSupportedOperatingSystems(attribute); + if (attributeOperatingSystems is null) { - return []; + continue; } if (supportedOperatingSystems is null) { - supportedOperatingSystems = conditionOperatingSystems; + supportedOperatingSystems = attributeOperatingSystems; } else { - supportedOperatingSystems.IntersectWith(conditionOperatingSystems); + supportedOperatingSystems.IntersectWith(attributeOperatingSystems); } } - if (supportedOperatingSystems is null || supportedOperatingSystems.Count == 0) - { - return []; - } - - return [CreateCapability(supportedOperatingSystems)]; + return supportedOperatingSystems is { Count: 0 }; } /// @@ -98,6 +97,46 @@ public static IReadOnlyList GetWorkerCapabilities(string operatingSystem return capabilities; } + [UnconditionalSuppressMessage( + "Trimming", + "IL2067", + Justification = "Condition types come from RunIfAll generic arguments, whose new() constraint preserves a public parameterless constructor.")] + private static HashSet? GetSupportedOperatingSystems(IConditionAttribute attribute) + { + if (attribute.Logic != ConditionLogic.All) + { + return null; + } + + var conditionTypes = attribute.GetType().GetGenericArguments(); + if (conditionTypes.Length == 0) + { + return null; + } + + HashSet? supportedOperatingSystems = null; + + foreach (var conditionType in conditionTypes) + { + var conditionOperatingSystems = GetSupportedOperatingSystems(conditionType); + if (conditionOperatingSystems is null) + { + return null; + } + + if (supportedOperatingSystems is null) + { + supportedOperatingSystems = conditionOperatingSystems; + } + else + { + supportedOperatingSystems.IntersectWith(conditionOperatingSystems); + } + } + + return supportedOperatingSystems; + } + [UnconditionalSuppressMessage( "Trimming", "IL2067", diff --git a/src/ModularPipelines/Configuration/ModuleConfigurationBuilder.cs b/src/ModularPipelines/Configuration/ModuleConfigurationBuilder.cs index 40aff23312..5008aed222 100644 --- a/src/ModularPipelines/Configuration/ModuleConfigurationBuilder.cs +++ b/src/ModularPipelines/Configuration/ModuleConfigurationBuilder.cs @@ -61,15 +61,13 @@ public sealed class ModuleConfigurationBuilder /// A function that receives the module context and returns a . /// This builder instance for method chaining. /// - /// Repeated conditions are evaluated in registration order and combined with AND-to-skip semantics: - /// the module is skipped only when every condition returns . - /// Any result keeps the module eligible to run. - /// To skip when any of several predicates matches, combine those predicates in one condition. + /// Repeated conditions use OR-to-skip semantics. Evaluation stops when a condition returns + /// . /// public ModuleConfigurationBuilder WithSkipWhen(Func condition) { ArgumentNullException.ThrowIfNull(condition); - _skipConditions.Add((context, _) => ValueTask.FromResult(condition(context))); + _skipConditions.Add(AdaptSkipCondition(condition)); return this; } @@ -79,10 +77,8 @@ public ModuleConfigurationBuilder WithSkipWhen(FuncA function that receives the module context and cancellation token and returns a . /// This builder instance for method chaining. /// - /// Repeated conditions are evaluated in registration order and combined with AND-to-skip semantics: - /// the module is skipped only when every condition returns . - /// Any result keeps the module eligible to run. - /// To skip when any of several predicates matches, combine those predicates in one condition. + /// Repeated conditions use OR-to-skip semantics. Evaluation stops when a condition returns + /// . /// public ModuleConfigurationBuilder WithSkipWhen( Func> condition) @@ -92,6 +88,43 @@ public ModuleConfigurationBuilder WithSkipWhen( return this; } + /// + /// Adds a synchronous group of conditions that must all return skip decisions to skip the module. + /// + /// Conditions evaluated in registration order with AND-to-skip semantics. + /// This builder instance for method chaining. + /// + /// Each call adds one AND group. The group composes with other skip conditions using OR-to-skip semantics. + /// + public ModuleConfigurationBuilder WithSkipWhenAll( + params Func[] conditions) + { + ArgumentNullException.ThrowIfNull(conditions); + ValidateSkipConditionGroup(conditions); + + _skipConditions.Add(ComposeAllSkipConditions( + Array.ConvertAll(conditions, AdaptSkipCondition))); + return this; + } + + /// + /// Adds an asynchronous group of conditions that must all return skip decisions to skip the module. + /// + /// Conditions evaluated in registration order with AND-to-skip semantics. + /// This builder instance for method chaining. + /// + /// Each call adds one AND group. The group composes with other skip conditions using OR-to-skip semantics. + /// + public ModuleConfigurationBuilder WithSkipWhenAll( + params Func>[] conditions) + { + ArgumentNullException.ThrowIfNull(conditions); + ValidateSkipConditionGroup(conditions); + + _skipConditions.Add(ComposeAllSkipConditions([.. conditions])); + return this; + } + #endregion #region Scheduling and Metadata @@ -371,6 +404,24 @@ internal ModuleConfigurationBuilder SetAdvancedRetryPolicy(Func + { + foreach (var condition in conditions) + { + var decision = await condition(context, cancellationToken).ConfigureAwait(false); + if (decision.ShouldSkip) + { + return decision; + } + } + + return SkipDecision.DoNotSkip; + }; + } + + private static Func> ComposeAllSkipConditions( + IReadOnlyList>> conditions) + { return async (context, cancellationToken) => { List? reasons = null; @@ -393,6 +444,26 @@ internal ModuleConfigurationBuilder SetAdvancedRetryPolicy(Func> AdaptSkipCondition( + Func condition) + { + return (context, _) => ValueTask.FromResult(condition(context)); + } + + private static void ValidateSkipConditionGroup(TCondition[] conditions) + where TCondition : Delegate + { + if (conditions.Length == 0) + { + throw new ArgumentException("At least one skip condition is required.", nameof(conditions)); + } + + if (conditions.Any(static condition => condition is null)) + { + throw new ArgumentException("Skip conditions cannot contain null values.", nameof(conditions)); + } + } + private static void ValidateModuleType(Type moduleType) { ArgumentNullException.ThrowIfNull(moduleType); diff --git a/src/ModularPipelines/Engine/IModuleConditionHandler.cs b/src/ModularPipelines/Engine/IModuleConditionHandler.cs index 2e8e072e7f..68f4b8b487 100644 --- a/src/ModularPipelines/Engine/IModuleConditionHandler.cs +++ b/src/ModularPipelines/Engine/IModuleConditionHandler.cs @@ -5,5 +5,9 @@ namespace ModularPipelines.Engine; internal interface IModuleConditionHandler { + Task<(bool ShouldIgnore, SkipDecision? SkipDecision)> ShouldIgnoreByCategory( + IModule module, + CancellationToken cancellationToken = default); + Task<(bool ShouldIgnore, SkipDecision? SkipDecision)> ShouldIgnore(IModule module, CancellationToken cancellationToken = default); -} \ No newline at end of file +} diff --git a/src/ModularPipelines/Engine/ModuleConditionHandler.cs b/src/ModularPipelines/Engine/ModuleConditionHandler.cs index 320953ccc2..2de07f8f39 100644 --- a/src/ModularPipelines/Engine/ModuleConditionHandler.cs +++ b/src/ModularPipelines/Engine/ModuleConditionHandler.cs @@ -62,9 +62,40 @@ public ModuleConditionHandler( } } + public Task<(bool ShouldIgnore, SkipDecision? SkipDecision)> ShouldIgnoreByCategory( + IModule module, + CancellationToken cancellationToken = default) + { + cancellationToken.ThrowIfCancellationRequested(); + var result = EvaluateCategoryConditions(module); + if (!result.ShouldIgnore + && IsDistributedMaster() + && OperatingSystemConditions.HasImpossibleCombination(GetConditionAttributes(module.GetType()).All)) + { + result = (true, SkipDecision.Skip("Module requires mutually exclusive operating systems")); + } + + return Task.FromResult(result); + } + private async Task<(bool ShouldIgnore, SkipDecision? SkipDecision)> EvaluateShouldIgnore( IModule module, CancellationToken cancellationToken) + { + var categoryResult = EvaluateCategoryConditions(module); + if (categoryResult.ShouldIgnore) + { + return categoryResult; + } + + var moduleType = module.GetType(); + var conditionResult = await IsRunnableCondition(moduleType, cancellationToken).ConfigureAwait(false); + return conditionResult.IsRunnable + ? (false, null) + : (true, conditionResult.SkipDecision); + } + + private (bool ShouldIgnore, SkipDecision? SkipDecision) EvaluateCategoryConditions(IModule module) { var moduleType = module.GetType(); _metadataRegistry.FinalizeMetadata(moduleType, module); @@ -80,10 +111,7 @@ public ModuleConditionHandler( return (true, SkipDecision.Skip("The module was not in a runnable category")); } - var conditionResult = await IsRunnableCondition(moduleType, cancellationToken).ConfigureAwait(false); - return conditionResult.IsRunnable - ? (false, null) - : (true, conditionResult.SkipDecision); + return (false, null); } private bool IsRunnableCategory(string? category) diff --git a/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs b/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs index 81aed8703c..0218fa0fc4 100644 --- a/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs +++ b/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs @@ -33,17 +33,20 @@ internal class ModuleExecutionPipeline : IModuleExecutionPipeline private readonly IModuleResultRepository _resultRepository; private readonly EngineCancellationToken _engineCancellationToken; private readonly IDirectHookInvoker _directHookInvoker; + private readonly IModuleConditionHandler _moduleConditionHandler; private readonly IOptions _pipelineOptions; public ModuleExecutionPipeline( IModuleResultRepository resultRepository, EngineCancellationToken engineCancellationToken, IDirectHookInvoker directHookInvoker, + IModuleConditionHandler moduleConditionHandler, IOptions pipelineOptions) { _resultRepository = resultRepository; _engineCancellationToken = engineCancellationToken; _directHookInvoker = directHookInvoker; + _moduleConditionHandler = moduleConditionHandler; _pipelineOptions = pipelineOptions; } @@ -67,8 +70,19 @@ public async Task> ExecuteAsync( // Setup cancellation based on AlwaysRun behavior SetupCancellation(config, executionContext, engineCancellationToken); - // A required dependency can skip after discovery through a fluent condition. + // A required dependency can skip before this module reaches its own conditions. var skipDecision = executionContext.SkipResult; + if (!skipDecision.ShouldSkip) + { + var (shouldIgnore, attributeSkipDecision) = await _moduleConditionHandler + .ShouldIgnore(module, executionContext.ModuleCancellationTokenSource.Token) + .ConfigureAwait(false); + if (shouldIgnore) + { + skipDecision = attributeSkipDecision ?? SkipDecision.Skip("Module was ignored"); + } + } + if (!skipDecision.ShouldSkip && config.SkipCondition != null) { skipDecision = await config.SkipCondition( diff --git a/src/ModularPipelines/Engine/ModuleRetriever.cs b/src/ModularPipelines/Engine/ModuleRetriever.cs index 5bdc35a11c..6e5a965cfc 100644 --- a/src/ModularPipelines/Engine/ModuleRetriever.cs +++ b/src/ModularPipelines/Engine/ModuleRetriever.cs @@ -99,7 +99,9 @@ private async Task DiscoverModules( continue; } - var (shouldIgnore, skipDecision) = await _moduleConditionHandler.ShouldIgnore(module, cancellationToken).ConfigureAwait(false); + var (shouldIgnore, skipDecision) = await _moduleConditionHandler + .ShouldIgnoreByCategory(module, cancellationToken) + .ConfigureAwait(false); if (shouldIgnore) { modulesToIgnore.Add(new IgnoredModule(module, skipDecision ?? SkipDecision.Skip("Module was ignored"))); diff --git a/test/ModularPipelines.UnitTests/Attributes/LifecycleEventIntegrationTests.cs b/test/ModularPipelines.UnitTests/Attributes/LifecycleEventIntegrationTests.cs index 924b58b7c5..33096cad24 100644 --- a/test/ModularPipelines.UnitTests/Attributes/LifecycleEventIntegrationTests.cs +++ b/test/ModularPipelines.UnitTests/Attributes/LifecycleEventIntegrationTests.cs @@ -1,6 +1,7 @@ using Microsoft.Extensions.DependencyInjection; using ModularPipelines.Attributes.Events; using ModularPipelines.Configuration; +using ModularPipelines.Conditions; using ModularPipelines.Context; using ModularPipelines.Models; using ModularPipelines.Modules; @@ -52,6 +53,11 @@ public Task OnModuleSkippedAsync(IModuleHookContext context, SkipDecision reason } } + public class AlwaysTrueCondition : IRunCondition + { + public Task EvaluateAsync(IPipelineContext context) => Task.FromResult(true); + } + [LogStart] [LogEnd] public class SuccessfulModule : Module @@ -87,6 +93,19 @@ protected override ModuleConfiguration Configure() => ModuleConfiguration.Create } } + [ModularPipelines.Attributes.SkipIf] + [LogStart] + [LogSkipped] + public class AttributeSkippingModule : Module + { + protected internal override Task ExecuteAsync( + IModuleContext context, + CancellationToken cancellationToken) + { + return Task.FromResult("Should not execute"); + } + } + [Before(Test)] public void ClearEventLog() { @@ -138,4 +157,16 @@ public async Task SkippingModule_InvokesStartAndSkippedEvents() await Assert.That(EventLog).Contains("Start:SkippingModule"); await Assert.That(EventLog.Any(e => e.Contains("Skipped:SkippingModule:Test skip reason"))).IsTrue(); } + + [Test] + public async Task AttributeSkippingModule_InvokesStartAndSkippedEvents() + { + var result = await TestPipelineHostBuilder.Create() + .AddModule() + .ExecutePipelineAsync(); + + await Assert.That(result.Status).IsEqualTo(Enums.Status.Successful); + await Assert.That(EventLog).Contains("Start:AttributeSkippingModule"); + await Assert.That(EventLog.Any(e => e.Contains("Skipped:AttributeSkippingModule:SkipIf returned true"))).IsTrue(); + } } diff --git a/test/ModularPipelines.UnitTests/Configuration/ModuleConfigurationTests.cs b/test/ModularPipelines.UnitTests/Configuration/ModuleConfigurationTests.cs index c5236c5e9a..e5487bcf38 100644 --- a/test/ModularPipelines.UnitTests/Configuration/ModuleConfigurationTests.cs +++ b/test/ModularPipelines.UnitTests/Configuration/ModuleConfigurationTests.cs @@ -92,19 +92,35 @@ await Assert.That(parameterTypes).IsEquivalentTo( } [Test] - public async Task WithSkipWhen_RepeatedCalls_AndComposeAndShortCircuit() + public async Task WithSkipWhenAll_ExposesSyncAndAsyncGroups() + { + var parameterTypes = typeof(ModuleConfigurationBuilder) + .GetMethods() + .Where(method => method.Name == nameof(ModuleConfigurationBuilder.WithSkipWhenAll)) + .Select(method => method.GetParameters().Single().ParameterType) + .ToArray(); + + await Assert.That(parameterTypes).IsEquivalentTo( + [ + typeof(Func[]), + typeof(Func>[]), + ]); + } + + [Test] + public async Task WithSkipWhen_RepeatedCalls_OrComposeAndShortCircuit() { var evaluatedConditions = new List(); var config = ModuleConfiguration.Create() .WithSkipWhen(_ => { evaluatedConditions.Add("first"); - return SkipDecision.DoNotSkip; + return SkipDecision.Skip("First reason"); }) .WithSkipWhen(_ => { evaluatedConditions.Add("second"); - return SkipDecision.Skip("Should not be evaluated"); + return SkipDecision.DoNotSkip; }) .Build(); @@ -112,11 +128,29 @@ public async Task WithSkipWhen_RepeatedCalls_AndComposeAndShortCircuit() using (Assert.Multiple()) { - await Assert.That(decision.ShouldSkip).IsFalse(); + await Assert.That(decision.ShouldSkip).IsTrue(); + await Assert.That(decision.Reason).IsEqualTo("First reason"); await Assert.That(evaluatedConditions).IsEquivalentTo(["first"]); } } + [Test] + public async Task WithSkipWhen_RepeatedCalls_SkipWhenLaterConditionMatches() + { + var config = ModuleConfiguration.Create() + .WithSkipWhen(_ => SkipDecision.DoNotSkip) + .WithSkipWhen(_ => SkipDecision.Skip("Second reason")) + .Build(); + + var decision = await config.SkipCondition!(Mock.Of(), CancellationToken.None); + + using (Assert.Multiple()) + { + await Assert.That(decision.ShouldSkip).IsTrue(); + await Assert.That(decision.Reason).IsEqualTo("Second reason"); + } + } + [Test] public async Task WithSkipWhen_SyncCondition_ReceivesContext() { @@ -179,11 +213,12 @@ public async Task WithSkipWhen_AsyncCondition_ReceivesCancellationToken() } [Test] - public async Task WithSkipWhen_AllConditionsSkip_CombinesReasons() + public async Task WithSkipWhenAll_AllConditionsSkip_CombinesReasons() { var config = ModuleConfiguration.Create() - .WithSkipWhen(_ => SkipDecision.Skip("First reason")) - .WithSkipWhen(_ => SkipDecision.Skip("Second reason")) + .WithSkipWhenAll( + _ => SkipDecision.Skip("First reason"), + _ => SkipDecision.Skip("Second reason")) .Build(); var decision = await config.SkipCondition!(Mock.Of(), CancellationToken.None); @@ -195,14 +230,74 @@ public async Task WithSkipWhen_AllConditionsSkip_CombinesReasons() } } + [Test] + public async Task WithSkipWhenAll_StopsWhenConditionDoesNotSkip() + { + var evaluatedConditions = new List(); + var config = ModuleConfiguration.Create() + .WithSkipWhenAll( + _ => + { + evaluatedConditions.Add("first"); + return SkipDecision.DoNotSkip; + }, + _ => + { + evaluatedConditions.Add("second"); + return SkipDecision.Skip("Should not be evaluated"); + }) + .Build(); + + var decision = await config.SkipCondition!(Mock.Of(), CancellationToken.None); + + using (Assert.Multiple()) + { + await Assert.That(decision.ShouldSkip).IsFalse(); + await Assert.That(evaluatedConditions).IsEquivalentTo(["first"]); + } + } + + [Test] + public async Task WithSkipWhenAll_SnapshotsAsyncConditionGroups() + { + Func>[] conditions = + [ + (_, _) => ValueTask.FromResult(SkipDecision.Skip("Original reason")), + ]; + var config = ModuleConfiguration.Create() + .WithSkipWhenAll(conditions) + .Build(); + + conditions[0] = (_, _) => ValueTask.FromResult(SkipDecision.DoNotSkip); + + var decision = await config.SkipCondition!( + Mock.Of(), + CancellationToken.None); + + using (Assert.Multiple()) + { + await Assert.That(decision.ShouldSkip).IsTrue(); + await Assert.That(decision.Reason).IsEqualTo("Original reason"); + } + } + + [Test] + public void WithSkipWhenAll_RejectsEmptyGroups() + { + var builder = ModuleConfiguration.Create(); + + Assert.Throws(() => + builder.WithSkipWhenAll(Array.Empty>())); + } + [Test] public async Task Build_SnapshotsSkipConditions() { var builder = ModuleConfiguration.Create() - .WithSkipWhen(_ => SkipDecision.Skip("First reason")); + .WithSkipWhen(_ => SkipDecision.DoNotSkip); var firstConfig = builder.Build(); - builder.WithSkipWhen(_ => SkipDecision.DoNotSkip); + builder.WithSkipWhen(_ => SkipDecision.Skip("Second reason")); var secondConfig = builder.Build(); var context = Mock.Of(); @@ -211,8 +306,8 @@ public async Task Build_SnapshotsSkipConditions() using (Assert.Multiple()) { - await Assert.That(firstDecision.ShouldSkip).IsTrue(); - await Assert.That(secondDecision.ShouldSkip).IsFalse(); + await Assert.That(firstDecision.ShouldSkip).IsFalse(); + await Assert.That(secondDecision.ShouldSkip).IsTrue(); } } diff --git a/test/ModularPipelines.UnitTests/Engine/ModuleConditionHandlerTests.cs b/test/ModularPipelines.UnitTests/Engine/ModuleConditionHandlerTests.cs index 202d7481c8..147f24bfd6 100644 --- a/test/ModularPipelines.UnitTests/Engine/ModuleConditionHandlerTests.cs +++ b/test/ModularPipelines.UnitTests/Engine/ModuleConditionHandlerTests.cs @@ -96,6 +96,36 @@ public async Task Distributed_Master_Filters_Module_With_Contradictory_Os_Condit await Assert.That(result.ShouldIgnore).IsTrue(); } + [Test] + public async Task Distributed_Master_Discovery_Filters_Module_With_Contradictory_Os_Conditions() + { + var handler = CreateHandler(new DistributedOptions + { + Enabled = true, + InstanceIndex = 0, + TotalInstances = 3, + }); + + var result = await handler.ShouldIgnoreByCategory(new ContradictoryOsModule()); + + await Assert.That(result.ShouldIgnore).IsTrue(); + } + + [Test] + public async Task Distributed_Master_Discovery_Does_Not_Filter_Routable_Os_Condition() + { + var handler = CreateHandler(new DistributedOptions + { + Enabled = true, + InstanceIndex = 0, + TotalInstances = 3, + }); + + var result = await handler.ShouldIgnoreByCategory(CreateForeignOsModule()); + + await Assert.That(result.ShouldIgnore).IsFalse(); + } + [Test] public async Task Distributed_Master_Does_Not_Filter_Unix_Condition_Group() { diff --git a/test/ModularPipelines.UnitTests/Engine/ModuleExecutionPipelineTests.cs b/test/ModularPipelines.UnitTests/Engine/ModuleExecutionPipelineTests.cs index 6b4a576474..9e9907e733 100644 --- a/test/ModularPipelines.UnitTests/Engine/ModuleExecutionPipelineTests.cs +++ b/test/ModularPipelines.UnitTests/Engine/ModuleExecutionPipelineTests.cs @@ -55,10 +55,15 @@ public async Task ExecuteAsync_DisposesOriginalAndLinkedCancellationTokenSources .ReturnsAsync((ModuleResult?) null); using var engineCancellationToken = new PipelineEngineCancellationToken(new PrimaryExceptionContainer()); + var moduleConditionHandler = new Mock(); + moduleConditionHandler + .Setup(x => x.ShouldIgnore(module, It.IsAny())) + .ReturnsAsync((false, null)); var pipeline = new ModuleExecutionPipeline( resultRepository.Object, engineCancellationToken, directHookInvoker.Object, + moduleConditionHandler.Object, OptionsFactory.Create(new PipelineOptions())); await pipeline.ExecuteAsync( diff --git a/test/ModularPipelines.UnitTests/Engine/ModuleRetrieverTests.cs b/test/ModularPipelines.UnitTests/Engine/ModuleRetrieverTests.cs index ac5e37523c..12f20a8bc5 100644 --- a/test/ModularPipelines.UnitTests/Engine/ModuleRetrieverTests.cs +++ b/test/ModularPipelines.UnitTests/Engine/ModuleRetrieverTests.cs @@ -17,7 +17,7 @@ public async Task EstimatedTimeLookups_AreNotRateLimited() .ToArray(); var conditionHandler = new Mock(); conditionHandler - .Setup(x => x.ShouldIgnore(It.IsAny(), It.IsAny())) + .Setup(x => x.ShouldIgnoreByCategory(It.IsAny(), It.IsAny())) .ReturnsAsync((false, null)); var registrationEventExecutor = new Mock(); registrationEventExecutor diff --git a/test/ModularPipelines.UnitTests/Execution/NewRunConditionAttributeTests.cs b/test/ModularPipelines.UnitTests/Execution/NewRunConditionAttributeTests.cs index 01188217d9..3f776fe861 100644 --- a/test/ModularPipelines.UnitTests/Execution/NewRunConditionAttributeTests.cs +++ b/test/ModularPipelines.UnitTests/Execution/NewRunConditionAttributeTests.cs @@ -34,11 +34,19 @@ private static bool SubsequentConditionWasEvaluated set => CurrentConditionState.SubsequentConditionWasEvaluated = value; } + private static bool DependencyWasExecuted + { + get => CurrentConditionState.DependencyWasExecuted; + set => CurrentConditionState.DependencyWasExecuted = value; + } + private sealed class ConditionState { public CancellationTokenSource? CancellationTokenSource { get; set; } public bool SubsequentConditionWasEvaluated { get; set; } + + public bool DependencyWasExecuted { get; set; } } #region Test Conditions @@ -211,14 +219,27 @@ private class ThrowOnConstruction : IRunCondition public Task EvaluateAsync(IPipelineContext context) => Task.FromResult(true); } - private class UnregisteredDependencyModule : SimpleTestModule + private class ConditionDependencyModule : SimpleTestModule { - protected override bool Result => true; + protected override bool Result + { + get + { + DependencyWasExecuted = true; + return true; + } + } } - [RunIfAll] - [ModularPipelines.Attributes.DependsOn] - private class SkippedModuleWithUnregisteredDependency : SimpleTestModule + private class DependencyCompletedCondition : IRunCondition + { + public Task EvaluateAsync(IPipelineContext context) + => Task.FromResult(DependencyWasExecuted); + } + + [SkipIf] + [ModularPipelines.Attributes.DependsOn] + private class ConditionAfterDependencyModule : SimpleTestModule { protected override bool Result => true; } @@ -494,17 +515,23 @@ public async Task Attribute_And_Fluent_Conditions_Use_One_Skip_Pipeline() } [Test] - public async Task Attribute_Condition_Is_Evaluated_Before_Dependencies() + public async Task Attribute_Condition_Is_Evaluated_After_Dependencies() { + DependencyWasExecuted = false; var host = await TestPipelineHostBuilder.Create() - .AddModule() + .AddModule() + .AddModule() .BuildAsync(); await host.RunAsync(); var resultRegistry = host.Services.GetRequiredService(); - var moduleResult = resultRegistry.GetResult(typeof(SkippedModuleWithUnregisteredDependency))!; - await Assert.That(moduleResult.ModuleStatus).IsEqualTo(Status.Skipped); + var moduleResult = resultRegistry.GetResult(typeof(ConditionAfterDependencyModule))!; + using (Assert.Multiple()) + { + await Assert.That(DependencyWasExecuted).IsTrue(); + await Assert.That(moduleResult.ModuleStatus).IsEqualTo(Status.Skipped); + } } [Test] diff --git a/test/ModularPipelines.UnitTests/Hooks/DirectModuleHooksTests.cs b/test/ModularPipelines.UnitTests/Hooks/DirectModuleHooksTests.cs index 46e65b8bb1..09211a7285 100644 --- a/test/ModularPipelines.UnitTests/Hooks/DirectModuleHooksTests.cs +++ b/test/ModularPipelines.UnitTests/Hooks/DirectModuleHooksTests.cs @@ -1,5 +1,6 @@ using Microsoft.Extensions.DependencyInjection; using ModularPipelines.Configuration; +using ModularPipelines.Conditions; using ModularPipelines.Context; using ModularPipelines.Engine; using ModularPipelines.Enums; @@ -97,6 +98,36 @@ protected override Task OnSkippedAsync( } } + private class AlwaysTrueCondition : IRunCondition + { + public Task EvaluateAsync(IPipelineContext context) => Task.FromResult(true); + } + + [ModularPipelines.Attributes.SkipIf] + private class AttributeSkippableHookTrackingModule : Module + { + public List HooksCalled { get; } = []; + public SkipDecision? ReceivedSkipDecision { get; private set; } + + protected internal override Task ExecuteAsync( + IModuleContext context, + CancellationToken cancellationToken) + { + HooksCalled.Add("ExecuteAsync"); + return Task.FromResult("Should not reach here"); + } + + protected override Task OnSkippedAsync( + IModuleContext context, + SkipDecision skipDecision, + CancellationToken cancellationToken) + { + HooksCalled.Add("OnSkippedAsync"); + ReceivedSkipDecision = skipDecision; + return Task.CompletedTask; + } + } + /// /// Module that fails and tracks OnFailedAsync. /// @@ -229,6 +260,18 @@ public async Task OnSkippedAsync_CalledWhenModuleSkipped() await Assert.That(module.ReceivedSkipDecision!.Reason).IsEqualTo("Test skip reason"); } + [Test] + public async Task OnSkippedAsync_CalledWhenAttributeSkipsModule() + { + var module = await RunModule(); + + await Assert.That(module.HooksCalled).Contains("OnSkippedAsync"); + await Assert.That(module.HooksCalled).DoesNotContain("ExecuteAsync"); + await Assert.That(module.ReceivedSkipDecision).IsNotNull(); + await Assert.That(module.ReceivedSkipDecision!.Reason) + .IsEqualTo("SkipIf returned true"); + } + [Test] public async Task OnFailedAsync_CalledWhenModuleFails() { diff --git a/test/ModularPipelines.UnitTests/Validation/ValidationTests.cs b/test/ModularPipelines.UnitTests/Validation/ValidationTests.cs index 5836ecc377..4e35841f14 100644 --- a/test/ModularPipelines.UnitTests/Validation/ValidationTests.cs +++ b/test/ModularPipelines.UnitTests/Validation/ValidationTests.cs @@ -120,7 +120,7 @@ private class NeverRun : IRunCondition } [RunIfAll] - private class SkippedModuleWithFluentMissingDependency : Module + private class ExecutionSkippedModuleWithFluentMissingDependency : Module { protected override ModuleConfiguration Configure() => ModuleConfiguration.Create() .DependsOn() @@ -315,26 +315,21 @@ public async Task BuildAsync_WithFluentMissingDependency_ThrowsValidationExcepti } [Test] - public async Task BuildAsync_Ignores_Fluent_Dependencies_For_Discovery_Skipped_Modules() + public async Task BuildAsync_Rejects_Fluent_Missing_Dependency_For_Execution_Skipped_Module() { var builder = Pipeline.CreateBuilder(); - builder.AddModule(); + builder.AddModule(); - await using var pipeline = await builder.BuildAsync(); - - await Assert.That(pipeline).IsNotNull(); + await Assert.ThrowsAsync(() => builder.BuildAsync()); } [Test] - public async Task ValidateAsync_Ignores_Fluent_Dependencies_For_Discovery_Skipped_Modules() + public async Task ValidateAsync_Rejects_Fluent_Missing_Dependency_For_Execution_Skipped_Module() { var builder = Pipeline.CreateBuilder(); - builder.AddModule(); + builder.AddModule(); - var result = await builder.ValidateAsync(); - - await Assert.That(result.Errors).DoesNotContain(error => - error.Category == ValidationErrorCategory.Dependency); + await AssertDependencyValidationError(builder); } [Test]