-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: Refactored the default activity invoker to handle exceptions #7426
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| using Elsa.Extensions; | ||
| using Elsa.Mediator.Contracts; | ||
| using Elsa.Testing.Shared; | ||
| using Elsa.Workflows.Attributes; | ||
| using Elsa.Workflows.CommitStates; | ||
| using Elsa.Workflows.Pipelines.ActivityExecution; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.DependencyInjection.Extensions; | ||
| using Microsoft.Extensions.Logging; | ||
| using NSubstitute; | ||
| using Xunit.Abstractions; | ||
|
|
||
| namespace Elsa.Workflows.Core.UnitTests.Middleware.Activities; | ||
|
|
||
| /// <summary> | ||
| /// Base class for testing activity execution middleware components. Provides a test activity and an | ||
| /// activity execution context that can be used in the tests. Also allows you to configure the | ||
| /// <see cref="IServiceCollection"/> for the tests by overriding the <see cref="ConfigureServices"/> method. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of the activity execution middleware being tested.</typeparam> | ||
| public abstract class ActivityExecutionMiddlewareTestsBase<T> : IAsyncLifetime where T : class, IActivityExecutionMiddleware | ||
| { | ||
| [Activity(Type = "TestActivity", Namespace = "UnitTests")] | ||
| public class TestActivity : Activity | ||
| { | ||
| public Exception? ExecuteThrows { get; set; } | ||
| public Exception? ExecuteFaults { get; set; } | ||
| public Exception? CanExecuteThrows { get; set; } | ||
| public bool AutoComplete { get; set; } = false; | ||
|
|
||
| protected override ValueTask<bool> CanExecuteAsync(ActivityExecutionContext context) | ||
| { | ||
| if (CanExecuteThrows is not null) | ||
| { | ||
| throw CanExecuteThrows; | ||
| } | ||
|
|
||
| return ValueTask.FromResult(true); | ||
| } | ||
|
|
||
| protected override async ValueTask ExecuteAsync(ActivityExecutionContext context) | ||
| { | ||
| if (ExecuteThrows is not null) | ||
| { | ||
| throw ExecuteThrows; | ||
| } | ||
|
|
||
| if (ExecuteFaults is not null) | ||
| { | ||
| context.Fault(ExecuteFaults); | ||
| } | ||
|
|
||
| if (AutoComplete) | ||
| { | ||
| await context.CompleteActivityAsync(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| protected readonly TestActivity _activity; | ||
| protected readonly ITestOutputHelper _testOutputHelper; | ||
| protected readonly INotificationSender _notificationSender; | ||
| private readonly ActivityTestFixture _activityTestFixture; | ||
|
|
||
| protected ActivityExecutionContext ExecutionContext { get; private set; } | ||
|
|
||
| protected Action<IActivityExecutionPipelineBuilder> PipelineFactory { get; set; } = b => b.UseMiddleware<T>(); | ||
|
|
||
| protected IActivityExecutionPipeline Pipeline => ExecutionContext.GetRequiredService<IActivityExecutionPipeline>(); | ||
|
|
||
| protected ActivityExecutionMiddlewareTestsBase(ITestOutputHelper testOutputHelper) | ||
| { | ||
| _testOutputHelper = testOutputHelper; | ||
| _activity = new TestActivity(); | ||
| _notificationSender = Substitute.For<INotificationSender>(); | ||
|
|
||
| ExecutionContext = default!; | ||
|
|
||
| _activityTestFixture = new ActivityTestFixture(_activity) | ||
| .ConfigureServices(ConfigureServices); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Allows you to configure the <see cref="IServiceCollection"/> | ||
| /// </summary> | ||
| protected virtual void ConfigureServices(IServiceCollection services) | ||
| { | ||
| // Skip doing any commits | ||
| var activityCommitStrategy = Substitute.For<IActivityCommitStrategy>(); | ||
| activityCommitStrategy | ||
| .ShouldCommit(Arg.Any<ActivityCommitStateStrategyContext>()) | ||
| .Returns(CommitAction.Skip); | ||
|
|
||
| services | ||
| .AddTransient<IActivityExecutionPipelineBuilder, ActivityExecutionPipelinePipelineBuilder>() | ||
| .AddTransient<IActivityExecutionMiddleware, T>() | ||
| .RemoveAll<INotificationSender>() | ||
| .AddTransient(_ => _notificationSender) | ||
| .AddTransient<IIncidentStrategyResolver, DefaultIncidentStrategyResolver>() | ||
| .AddTransient(_ => Substitute.For<ICommitStrategyRegistry>()) | ||
| .Configure<CommitStateOptions>(options => options.DefaultActivityCommitStrategy = activityCommitStrategy) | ||
| .AddTransient<IActivityExecutionPipeline>(sp => new ActivityExecutionPipeline(sp, PipelineFactory)) | ||
| .AddSingleton(_activity) | ||
| .AddLogging(config => config.AddProvider(new XunitLoggerProvider(_testOutputHelper))) | ||
| .RemoveAll<IActivityRegistry>() | ||
| .AddSingleton<ActivityRegistry>() | ||
| .AddSingleton<IActivityRegistry>(sp => | ||
| { | ||
| var registry = sp.GetRequiredService<ActivityRegistry>(); | ||
| registry.Add(_activity.GetType(), new Workflows.Models.ActivityDescriptor()); | ||
| return registry; | ||
| }); | ||
| } | ||
|
|
||
|
|
||
| async Task IAsyncLifetime.InitializeAsync() | ||
| { | ||
| ExecutionContext = await _activityTestFixture.BuildAsync(); | ||
| } | ||
|
|
||
| Task IAsyncLifetime.DisposeAsync() | ||
| => Task.CompletedTask; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| using System.ComponentModel; | ||
| using Elsa.Mediator.Contracts; | ||
| using NSubstitute; | ||
| using Xunit.Abstractions; | ||
|
|
||
| namespace Elsa.Workflows.Core.UnitTests.Middleware.Activities; | ||
|
|
||
| /// <summary> | ||
| /// Abstract class used to test activity execution middleware components. | ||
| /// It provides a set of standard tests that validate the behavior of the middleware component when executing an activity, such as: | ||
| /// </summary> | ||
| /// <typeparam name="T"></typeparam> | ||
| public abstract class ActivityInvokerMiddlewareTestsBase<T> : ActivityExecutionMiddlewareTestsBase<T> where T : class, IActivityExecutionMiddleware | ||
| { | ||
| protected ActivityInvokerMiddlewareTestsBase(ITestOutputHelper testOutputHelper) : base(testOutputHelper) | ||
| { | ||
| } | ||
|
|
||
|
|
||
| [Fact] | ||
| public async Task ActivityExceuteThrows_IncidentsCount_IsOne() | ||
| { | ||
| // Setup | ||
|
ByronMayne marked this conversation as resolved.
|
||
| _activity.ExecuteThrows = new Exception("EXCEPTION!"); | ||
|
|
||
| // Act | ||
|
ByronMayne marked this conversation as resolved.
|
||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| Assert.Single(ExecutionContext.WorkflowExecutionContext.Incidents); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task ActivityExceuteThrows_ActivityStatus_IsFaulted() | ||
| { | ||
| // Setup | ||
| _activity.ExecuteThrows = new Exception("EXCEPTION!"); | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| Assert.Equal(ActivityStatus.Faulted, ExecutionContext.Status); | ||
| Assert.Equal(1, ExecutionContext.AggregateFaultCount); | ||
| } | ||
|
Comment on lines
+33
to
+45
Contributor
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.
The Prompt To Fix With AIThis is a comment left during a code review.
Path: test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityInvokerMiddlewareTestsBase.cs
Line: 33-47
Comment:
**Incomplete `[Description]` and overly-broad assertion**
The `[Description]` text is cut off mid-sentence. More importantly, the assertion `DidNotReceive().SendAsync(Arg.Any<INotification>())` covers *all* notifications. `DefaultActivityInvokerMiddleware` sends an `ActivityCompleted` notification whenever `activityDidComplete` is `true`, which is the case here with `AutoComplete = true`. If the mock matches that call (same `CancellationToken` path), this test will fail. If it doesn't match (e.g. due to a `CancellationToken` value mismatch), the test passes vacuously without verifying the intended constraint. The description should be completed and the assertion scoped to the specific event types that shouldn't be raised by this middleware (e.g. `ActivityExecuting` / `ActivityExecuted`).
How can I resolve this? If you propose a fix, please make it concise.
ByronMayne marked this conversation as resolved.
|
||
|
|
||
| [Fact] | ||
| public async Task ActivityExecuteFaults_ActivityStatus_IsFualted() | ||
| { | ||
| // Setup | ||
| _activity.ExecuteFaults = new Exception("EXCEPTION!"); | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| Assert.Equal(ActivityStatus.Faulted, ExecutionContext.Status); | ||
| Assert.Equal(1, ExecutionContext.AggregateFaultCount); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task ActivityCanExceuteThrows_IncidentsCount_IsOne() | ||
| { | ||
| // Setup | ||
| _activity.CanExecuteThrows = new Exception("EXCEPTION!"); | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| Assert.Single(ExecutionContext.WorkflowExecutionContext.Incidents); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task ActivityCanExceuteThrows_ActivityStatus_IsFaulted() | ||
| { | ||
| // Setup | ||
| _activity.CanExecuteThrows = new Exception("EXCEPTION!"); | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| Assert.Equal(ActivityStatus.Faulted, ExecutionContext.Status); | ||
| } | ||
|
|
||
|
Comment on lines
+20
to
+86
Contributor
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.
Prompt To Fix With AIThis is a comment left during a code review.
Path: test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityInvokerMiddlewareTestsBase.cs
Line: 20-87
Comment:
**No test coverage for `ExecuteFaults` (direct-fault) path**
`TestActivity` exposes an `ExectueFaults` property (which calls `context.Fault()` directly without throwing), but `ActivityInvokerMiddlewareTestsBase` only tests the exception-throwing paths. The PR's stated goal is to make the exception-thrown and direct-fault paths *consistent*, yet there is no test that verifies the incident count and status are correct when `ExectueFaults` is set. Without this, a regression on the fault path would go undetected.
How can I resolve this? If you propose a fix, please make it concise. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| using Elsa.Workflows.Middleware.Activities; | ||
| using Xunit.Abstractions; | ||
|
|
||
| namespace Elsa.Workflows.Core.UnitTests.Middleware.Activities; | ||
|
|
||
| /// <summary> | ||
| /// Validates the behavior of the <see cref="DefaultActivityInvokerMiddleware"/> component by using the standard tests | ||
| /// from <see cref="ActivityInvokerMiddlewareTestsBase{T}"/>. | ||
| /// </summary> | ||
| public class DefaultActivityInvokerMiddlewareTests : ActivityInvokerMiddlewareTestsBase<DefaultActivityInvokerMiddleware> | ||
| { | ||
| public DefaultActivityInvokerMiddlewareTests(ITestOutputHelper testOutputHelper) : base(testOutputHelper) | ||
| { | ||
|
|
||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| using Elsa.Workflows.Middleware.Activities; | ||
| using Elsa.Workflows.Notifications; | ||
| using Elsa.Workflows.Pipelines.ActivityExecution; | ||
| using NSubstitute; | ||
| using Xunit.Abstractions; | ||
|
|
||
| namespace Elsa.Workflows.Core.UnitTests.Middleware.Activities; | ||
|
|
||
| public class NotificationPublishingMiddlewareTests : ActivityExecutionMiddlewareTestsBase<NotificationPublishingMiddleware> | ||
| { | ||
| public NotificationPublishingMiddlewareTests(ITestOutputHelper testOutputHelper) : base(testOutputHelper) | ||
| { | ||
| PipelineFactory += b => b.UseMiddleware<DefaultActivityInvokerMiddleware>(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task InvokeAsync_Raises_ActivityExecutingEvent() | ||
| { | ||
|
ByronMayne marked this conversation as resolved.
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
ByronMayne marked this conversation as resolved.
|
||
|
|
||
| // Assert | ||
| await _notificationSender.Received().SendAsync(Arg.Any<ActivityExecuting>()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task InvokeAsync_Raises_ActivityExecutedEvent() | ||
| { | ||
|
ByronMayne marked this conversation as resolved.
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| await _notificationSender.Received().SendAsync(Arg.Any<ActivityExecuted>()); | ||
|
Comment on lines
+18
to
+33
Contributor
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.
Prompt To Fix With AIThis is a comment left during a code review.
Path: test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/NotificationPublishingMiddlewareTests.cs
Line: 18-33
Comment:
**Typos in test method names**
`InvokeAsync_Rasies_ActivityExecutingEvent` and `InvokeAsync_Rasies_ActivityExecutedEvent` contain the misspelling "Rasies" (should be "Raises"). While this doesn't affect test execution, it makes the test suite harder to read.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.