Add subflow and task extension test mappings #11
Conversation
Introduces new extension mappings and configuration files for subflow and task test scenarios, including child and parent data extensions, HTTP data and transform extensions, and related workflow/view/test definitions. These additions support testing of longpolling, data enrichment, and error boundary handling in workflow extensions.
Reviewer's GuideAdds comprehensive test coverage for workflow extensions, subflow view/extension interactions, and error boundary behaviors by introducing new extension mappings, subflow parent/child workflows and views, and dedicated tasks/workflows for HTTP-based data/transform extensions and error handling scenarios, plus minor tweaks to existing task-test mappings and configs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR introduces comprehensive test infrastructure for subflow workflows and error boundary handling, including parent/child workflow definitions, data extensions with HTTP mapping capabilities, multiple error-level boundary scenarios (task, state, subflow), new task definitions for error simulation, and updates to test collections and mock API configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ParentWF as Parent Workflow
participant Extension as Data Extension
participant ChildWF as Child Workflow
participant ParentComplete
Client->>ParentWF: Start Parent Workflow
activate ParentWF
ParentWF->>Extension: Load Parent Data Extension
activate Extension
Extension-->>ParentWF: Extension Data Ready
deactivate Extension
ParentWF->>ChildWF: Start Child Subflow
activate ChildWF
ChildWF->>Extension: Load Child Data Extension
activate Extension
Extension-->>ChildWF: Extension Data Ready
deactivate Extension
rect rgb(200, 220, 255)
Note over ChildWF: Child Workflow Executes
ChildWF->>ChildWF: Process: child-active → child-completed
end
ChildWF-->>ParentWF: Subflow Completion Result
deactivate ChildWF
ParentWF->>ParentComplete: Execute Completion Mapping
activate ParentComplete
ParentComplete-->>ParentWF: Final Result with Test Data
deactivate ParentComplete
ParentWF-->>Client: Parent Workflow Completed
deactivate ParentWF
sequenceDiagram
participant Client
participant Workflow as Error Boundary Workflow
participant HttpTask as HTTP Task
participant ErrorHandler as Error Boundary Handler
participant Recovery as Error Recovery
Client->>Workflow: Start Workflow
activate Workflow
rect rgb(255, 240, 200)
Note over Workflow, HttpTask: Task-Level Error Boundary
Workflow->>HttpTask: Execute Task (503 Retry Scenario)
HttpTask-->>ErrorHandler: Error Response
ErrorHandler->>ErrorHandler: Evaluate Retry Policy
ErrorHandler->>HttpTask: Retry with Exponential Backoff
HttpTask-->>ErrorHandler: Success Response
end
rect rgb(255, 200, 200)
Note over Workflow, ErrorHandler: State-Level Error Boundary
Workflow->>ErrorHandler: Enter Error-Sensitive State
ErrorHandler->>ErrorHandler: Evaluate Error Code (400)
ErrorHandler->>ErrorHandler: Apply Ignore Action
end
rect rgb(220, 200, 255)
Note over Workflow, Recovery: Subflow Error Propagation
Workflow->>HttpTask: Invoke Subflow Task
HttpTask-->>ErrorHandler: Error in Subflow
ErrorHandler->>Recovery: Propagate to Parent
Recovery-->>Workflow: Recovery Action Executed
end
Workflow-->>Client: Workflow Completed with Recovery
deactivate Workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (49)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the new HTTP extension mappings (e.g. HttpTransformExtensionMapping and HttpDataExtensionMapping),
context.Instance.Id.ToString()is used without a null check when building headers, which can throw if the instance is null; consider using the same null-safe pattern (context.Instance?.Id) you used elsewhere or guarding earlier. - Several
OutputHandler/InputHandlermethods are declaredasyncbut never useawait(e.g. in many of the new mapping classes), which produces unnecessary state machines; consider removingasyncand returningTask.FromResult(...)where applicable for simpler and slightly more efficient code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new HTTP extension mappings (e.g. HttpTransformExtensionMapping and HttpDataExtensionMapping), `context.Instance.Id.ToString()` is used without a null check when building headers, which can throw if the instance is null; consider using the same null-safe pattern (`context.Instance?.Id`) you used elsewhere or guarding earlier.
- Several `OutputHandler`/`InputHandler` methods are declared `async` but never use `await` (e.g. in many of the new mapping classes), which produces unnecessary state machines; consider removing `async` and returning `Task.FromResult(...)` where applicable for simpler and slightly more efficient code.
## Individual Comments
### Comment 1
<location> `core/Extensions/task-test/src/HttpTransformExtensionMapping.csx:41-49` </location>
<code_context>
+ ["Content-Type"] = "application/json",
+ ["X-Extension-Name"] = "test-http-data-extension",
+ ["X-Request-Id"] = Guid.NewGuid().ToString(),
+ ["X-Correlation-Id"] = context.Instance.Id.ToString()
+ };
+ httpTask.SetHeaders(headers);
</code_context>
<issue_to_address>
**suggestion:** Guard against null Instance when building correlation headers to avoid brittle tests
In these workflow tests, a null `Instance` or `Instance.Id` will cause a `NullReferenceException` during setup instead of a clear, assertion-based failure. To keep tests robust (especially with partially initialized instances), consider a null-safe correlation header, e.g.:
```csharp
["X-Correlation-Id"] = context.Instance?.Id?.ToString() ?? Guid.NewGuid().ToString()
```
```suggestion
// Set headers
var correlationId = context.Instance?.Id.ToString() ?? Guid.NewGuid().ToString();
var headers = new Dictionary<string, string?>
{
["Content-Type"] = "application/json",
["X-Extension-Name"] = "test-http-transform-extension",
["X-Transform-Type"] = "enrichment",
["X-Request-Id"] = Guid.NewGuid().ToString(),
["X-Correlation-Id"] = correlationId
};
```
</issue_to_address>
### Comment 2
<location> `core/Extensions/task-test/src/HttpDataExtensionMapping.csx:35-43` </location>
<code_context>
+ ["Content-Type"] = "application/json",
+ ["X-Extension-Name"] = "test-http-data-extension",
+ ["X-Request-Id"] = Guid.NewGuid().ToString(),
+ ["X-Correlation-Id"] = context.Instance.Id.ToString()
+ };
+ httpTask.SetHeaders(headers);
</code_context>
<issue_to_address>
**suggestion:** Align correlation-id handling with other test mappings for consistency and null-safety
This mapping assumes `context.Instance` and `context.Instance.Id` are always non-null, so tests that start with minimal or malformed data will fail with a null reference before they can assert extension behavior. Using `context.Instance?.Id?.ToString()` with an appropriate fallback would avoid this and match the defensive pattern used elsewhere in the PR.
```suggestion
// Set headers
var correlationId = context.Instance?.Id?.ToString() ?? string.Empty;
var headers = new Dictionary<string, string?>
{
["Content-Type"] = "application/json",
["X-Extension-Name"] = "test-http-data-extension",
["X-Request-Id"] = Guid.NewGuid().ToString(),
["X-Correlation-Id"] = correlationId
};
httpTask.SetHeaders(headers);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary of ChangesHello @yilmaztayfun, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the testing capabilities for workflow extensions and error handling mechanisms. It provides dedicated test setups for validating long-polling behavior in subflow views, ensuring data enrichment and transformation via HTTP extensions, and thoroughly exercising error boundary configurations at global, task, and state levels, including retry policies and timeout handling. The changes aim to enhance the reliability and robustness of the workflow engine's extension and error management features. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of tests for subflows, task extensions, and error boundary handling. The additions of new workflow definitions, C# script mappings, and test configurations are well-structured. I've identified a few areas for improvement, primarily concerning potential null reference exceptions in some of the new C# scripts, along with some suggestions for enhancing code quality and performance. Overall, this is a valuable addition for improving test coverage.
| ["Content-Type"] = "application/json", | ||
| ["X-Extension-Name"] = "test-http-data-extension", | ||
| ["X-Request-Id"] = Guid.NewGuid().ToString(), | ||
| ["X-Correlation-Id"] = context.Instance.Id.ToString() |
There was a problem hiding this comment.
There is a potential NullReferenceException here. context.Instance could be null, which would cause a crash. You've used the null-conditional operator ?. elsewhere in this file (e.g., line 28), and the same defensive approach should be applied here to safely access the Id property.
["X-Correlation-Id"] = context.Instance?.Id.ToString()
| ["X-Extension-Name"] = "test-http-transform-extension", | ||
| ["X-Transform-Type"] = "enrichment", | ||
| ["X-Request-Id"] = Guid.NewGuid().ToString(), | ||
| ["X-Correlation-Id"] = context.Instance.Id.ToString() |
There was a problem hiding this comment.
There is a potential NullReferenceException here. context.Instance could be null, which would cause a crash. You should use the null-conditional operator ?. to safely access the Id property, similar to how it's used in other parts of the code.
["X-Correlation-Id"] = context.Instance?.Id.ToString()
| // Pass parent data to SubFlow | ||
| return Task.FromResult(new ScriptResponse | ||
| { | ||
| Data = context.Instance.Data, |
There was a problem hiding this comment.
There is a potential NullReferenceException here. context.Instance can be null, which would cause a crash when trying to access the Data property. You should use the null-conditional operator ?. for safe access.
Data = context.Instance?.Data,
| public async Task<bool> Handler(ScriptContext context) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The async keyword is unnecessary here as the method returns a value synchronously. Using async creates a state machine which adds a small performance overhead. For better performance and cleaner code, you can remove the async keyword and return a completed task directly using Task.FromResult.
public Task<bool> Handler(ScriptContext context)
{
return Task.FromResult(true);
}
| { | ||
| LogInformation("ChildCompleteMapping - Completing child workflow"); | ||
|
|
||
| var inputData = context.Instance?.Data; |
There was a problem hiding this comment.
The local variable inputData is declared but its value is never used. It should be removed to improve code clarity.
| { | ||
| LogInformation("ParentStartMapping - Initializing parent workflow"); | ||
|
|
||
| var inputData = context.Instance?.Data; |
There was a problem hiding this comment.
The local variable inputData is declared but its value is never used. It should be removed to improve code clarity.
| using System.Threading.Tasks; | ||
| using BBT.Workflow.Definitions; | ||
| using BBT.Workflow.Scripting; | ||
| using BBT.Workflow.Scripting;using BBT.Workflow.Scripting.Functions; |
There was a problem hiding this comment.
For better readability and to adhere to standard C# coding conventions, using statements should each be on their own line.
using BBT.Workflow.Scripting;
using BBT.Workflow.Scripting.Functions;
| public async Task<bool> Handler(ScriptContext context) | ||
| { | ||
| // Check if there was an error in the SubFlow | ||
| var data = context.Instance?.Data; | ||
|
|
||
| // If there's a subFlow error result, this transition should fire | ||
| if (data?.subFlowErrorResult != null) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
The async keyword is not necessary for this method since it returns a value synchronously. Using async generates a state machine, which introduces a slight performance overhead. To make the code cleaner and more performant, you can remove async and use Task.FromResult to return a completed task.
public Task<bool> Handler(ScriptContext context)
{
// Check if there was an error in the SubFlow
var data = context.Instance?.Data;
// If there's a subFlow error result, this transition should fire
if (data?.subFlowErrorResult != null)
{
return Task.FromResult(true);
}
return Task.FromResult(false);
}
Introduces new extension mappings and configuration files for subflow and task test scenarios, including child and parent data extensions, HTTP data and transform extensions, and related workflow/view/test definitions. These additions support testing of longpolling, data enrichment, and error boundary handling in workflow extensions.
Summary by Sourcery
Add workflow, task, and extension configurations to exercise HTTP-based data/transform extensions, subflow view/extension long-polling, and error boundary behaviors across parent/child workflows.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.