feat: add jmeter load test and account-opening instance lookup - #15
Merged
Conversation
Add a containerized JMeter load test for the async account-opening happy path that polls state between transitions, with overridable base URL and load profile. Also introduce get-instance(s) tasks and an initiate-account-opening schema to support querying existing instances and validating the start transition payload.
Reviewer's GuideAdds a containerized JMeter-based asynchronous account-opening load test and introduces account-opening workflow instance lookup tasks and schema refinements, plus minor cleanup. Sequence diagram for asynchronous account-opening JMeter load testsequenceDiagram
actor Tester
participant DockerCompose
participant JMeter as JMeter_container
participant Engine as Workflow_engine
Tester->>DockerCompose: docker compose -f docker-compose.test.yml up
DockerCompose->>JMeter: start container with env\nVNEXT_BASE_URL, JMETER_USERS, JMETER_RAMPUP, JMETER_LOOPS
JMeter->>JMeter: load workflow-test.jmx
loop For each virtual user and loop iteration
JMeter->>Engine: POST /instances/start?sync=false\nbody: initiate-account-opening payload
Engine-->>JMeter: 202 Accepted, instance id
loop Poll workflow state until status A or max polls
JMeter->>Engine: GET /instances/{id}
Engine-->>JMeter: instance status (B, A, C, F)
end
alt status A
JMeter->>Engine: PATCH /transitions/select-demand-deposit?sync=false
Engine-->>JMeter: 202 Accepted
JMeter->>Engine: poll instance until status A
Engine-->>JMeter: status A
JMeter->>Engine: PATCH /transitions/submit-account-details?sync=false
Engine-->>JMeter: 202 Accepted
JMeter->>Engine: poll instance until status A
Engine-->>JMeter: status A
JMeter->>Engine: PATCH /transitions/confirm-account-opening?sync=false
Engine-->>JMeter: final response (not asserted)
else status F or unexpected C
JMeter->>JMeter: mark iteration failed\nskip remaining samplers
end
end
JMeter-->>Tester: result.jtl and HTML report in jmeter/results
Class diagram for new account-opening instance lookup mappingsclassDiagram
class IMapping {
<<interface>>
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class WorkflowTask {
}
class GetInstancesTask {
+void SetFilter(object filter)
}
class GetInstanceDataTask {
+void SetInstance(string instanceId)
}
class ScriptContext {
+dynamic Body
}
class ScriptResponse {
+string Key
+object Data
}
class GetInstancesByOldKeyMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class GetInstanceByOldKeyMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
IMapping <|.. GetInstancesByOldKeyMapping
IMapping <|.. GetInstanceByOldKeyMapping
WorkflowTask <|-- GetInstancesTask
WorkflowTask <|-- GetInstanceDataTask
GetInstancesByOldKeyMapping --> GetInstancesTask : casts task
GetInstanceByOldKeyMapping --> GetInstanceDataTask : casts task
GetInstancesByOldKeyMapping --> ScriptContext : reads Body.oldKey
GetInstanceByOldKeyMapping --> ScriptContext : reads Body.oldKey
GetInstancesByOldKeyMapping --> ScriptResponse : returns
GetInstanceByOldKeyMapping --> ScriptResponse : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughIntroduces comprehensive load testing infrastructure for the account-opening workflow via JMeter, adds new task definitions and workflow mappings for instance retrieval, introduces a new workflow initiation schema, updates workflow task orchestration, and includes supporting configuration and documentation changes. Changes
Sequence Diagram(s)sequenceDiagram
participant JMeter as JMeter Client
participant API as API Server
participant WF as Workflow Engine
participant DB as Data Layer
JMeter->>API: POST /account-opening/start<br/>(initiate workflow)
API->>WF: Start workflow instance
WF->>DB: Create instance
DB-->>WF: Instance created
WF-->>API: instanceId
API-->>JMeter: instanceId, status=A
JMeter->>API: PATCH /account-opening/:id<br/>(state transition 1)
API->>WF: Execute transition (get-instances)
WF->>DB: Query instances by oldKey
DB-->>WF: Instances list
WF-->>API: Status updated
API-->>JMeter: Status updated, status=B
JMeter->>API: GET /account-opening/:id<br/>(poll state)
API->>WF: Get instance state
WF->>DB: Fetch instance
DB-->>WF: Instance data
WF-->>API: Current state
API-->>JMeter: status=B
Note over JMeter: Poll loop: sleep if status=B,<br/>continue until F/C or pollCount=5
JMeter->>API: PATCH /account-opening/:id<br/>(state transition 2)
API->>WF: Execute transition (get-instance)
WF->>DB: Get instance by oldKey
DB-->>WF: Instance data
WF-->>API: Status updated
API-->>JMeter: Status updated, status=C
JMeter->>JMeter: Exit poll loop (status=C)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
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 1 issue, and left some high level feedback:
- In both
GetInstancesByOldKeyMappingandGetInstanceByOldKeyMapping, avoid the null-forgiving cast ((task as ...)!); instead, check the task type and handle unexpected types gracefully to prevent runtimeNullReferenceExceptions. - Consider explicitly handling a missing or null
oldKeyin the instance lookup mappings (e.g., returning an empty result or error) rather than passing it straight intoSetFilter/SetInstance, as the current behavior withnullmay be ambiguous. - In
docker-compose.test.yml, pinning the JMeter image to a specific version instead ofalpine/jmeter:latestwould make the load test environment more deterministic across runs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both `GetInstancesByOldKeyMapping` and `GetInstanceByOldKeyMapping`, avoid the null-forgiving cast (`(task as ...)!`); instead, check the task type and handle unexpected types gracefully to prevent runtime `NullReferenceException`s.
- Consider explicitly handling a missing or null `oldKey` in the instance lookup mappings (e.g., returning an empty result or error) rather than passing it straight into `SetFilter`/`SetInstance`, as the current behavior with `null` may be ambiguous.
- In `docker-compose.test.yml`, pinning the JMeter image to a specific version instead of `alpine/jmeter:latest` would make the load test environment more deterministic across runs.
## Individual Comments
### Comment 1
<location path="docker-compose.test.yml" line_range="6-9" />
<code_context>
+
+services:
+ jmeter:
+ image: alpine/jmeter:latest
+ container_name: jmeter
+ networks:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `latest` JMeter image tag can introduce non-deterministic test behavior over time.
Please pin this to a specific JMeter version (for example `alpine/jmeter:5.6.3`) instead of `latest` to keep CI and local runs reproducible and avoid unexpected changes when the upstream image updates.
```suggestion
jmeter:
# Pin JMeter image version to keep CI and local runs reproducible
image: alpine/jmeter:5.6.3
container_name: jmeter
networks:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+6
to
+9
| jmeter: | ||
| image: alpine/jmeter:latest | ||
| container_name: jmeter | ||
| networks: |
There was a problem hiding this comment.
suggestion (bug_risk): Using latest JMeter image tag can introduce non-deterministic test behavior over time.
Please pin this to a specific JMeter version (for example alpine/jmeter:5.6.3) instead of latest to keep CI and local runs reproducible and avoid unexpected changes when the upstream image updates.
Suggested change
| jmeter: | |
| image: alpine/jmeter:latest | |
| container_name: jmeter | |
| networks: | |
| jmeter: | |
| # Pin JMeter image version to keep CI and local runs reproducible | |
| image: alpine/jmeter:5.6.3 | |
| container_name: jmeter | |
| networks: |
![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 JMeter-based load testing suite for the account-opening workflow, including a new docker-compose configuration, a detailed test plan, and updated documentation. It also adds new workflow tasks and C# script mappings for instance retrieval, alongside minor schema adjustments and code cleanups. Review feedback highlights critical authorization and identity inconsistencies in the JMeter test plan headers and recommends pinning the JMeter Docker image version for stability. Furthermore, suggestions were made to add defensive null checks in the new mapping scripts and to replace thread-blocking sleep calls in the test plan with native JMeter timers to avoid skewing performance results.
| <elementProp name="" elementType="Header"><stringProp name="Header.name">user_reference</stringProp><stringProp name="Header.value">34987491018</stringProp></elementProp> | ||
| <elementProp name="" elementType="Header"><stringProp name="Header.name">sub</stringProp><stringProp name="Header.value">34987491018</stringProp></elementProp> | ||
| <elementProp name="" elementType="Header"><stringProp name="Header.name">act_sub</stringProp><stringProp name="Header.value">34987491780</stringProp></elementProp> | ||
| <elementProp name="" elementType="Header"><stringProp name="Header.name">role</stringProp><stringProp name="Header.value">morph-idm.viewers</stringProp></elementProp> |
There was a problem hiding this comment.
The role morph-idm.viewers (plural) does not match the role morph-idm.viewer (singular) defined in the workflow configuration (account-opening-workflow.json, line 53). This mismatch will likely cause authorization failures during the load test.
<elementProp name="" elementType="Header"><stringProp name="Header.name">role</stringProp><stringProp name="Header.value">morph-idm.viewer</stringProp></elementProp>
Comment on lines
+154
to
+157
| <collectionProp name="HeaderManager.headers"> | ||
| <elementProp name="" elementType="Header"><stringProp name="Header.name">sub</stringProp><stringProp name="Header.value">349874917801</stringProp></elementProp> | ||
| <elementProp name="" elementType="Header"><stringProp name="Header.name">act_sub</stringProp><stringProp name="Header.value">34987491781</stringProp></elementProp> | ||
| </collectionProp> |
There was a problem hiding this comment.
The GET state request headers are missing the role header required by the workflow's queryRoles configuration. Additionally, the sub and act_sub values are inconsistent with those used in the Start Instance sampler (e.g., 349874917801 vs 34987491018), which may lead to permission issues if the workflow engine enforces identity consistency.
Comment on lines
+12
to
+16
| var instanceTask = (task as GetInstanceDataTask)!; | ||
|
|
||
| string? oldKey = context.Body?.oldKey?.ToString(); | ||
|
|
||
| instanceTask.SetInstance(oldKey); |
There was a problem hiding this comment.
The mapping lacks proper validation for the input task and the oldKey value. If the task cast fails or oldKey is missing from the body, the script will either throw a NullReferenceException or perform an invalid lookup. It is recommended to add defensive checks.
var instanceTask = task as GetInstanceDataTask;
if (instanceTask == null)
{
return Task.FromResult(new ScriptResponse { Key = "error", Data = "Invalid task type" });
}
string? oldKey = context.Body?.oldKey?.ToString();
if (string.IsNullOrEmpty(oldKey))
{
return Task.FromResult(new ScriptResponse { Key = "error", Data = "oldKey is required" });
}
instanceTask.SetInstance(oldKey);
|
|
||
| services: | ||
| jmeter: | ||
| image: alpine/jmeter:latest |
There was a problem hiding this comment.
Using the latest tag for the JMeter image can lead to non-deterministic builds and potential breaking changes when the image is updated. It is recommended to pin a specific version (e.g., 5.6) to ensure reproducibility.
image: alpine/jmeter:5.6| <stringProp name="script">def c = (vars.get("pollCount") as Integer) + 1 | ||
| vars.put("pollCount", c.toString()) | ||
| def s = vars.get("status") | ||
| if ("B".equals(s)) { Thread.sleep(1000) } |
There was a problem hiding this comment.
Using Thread.sleep() in a JSR223 PostProcessor is not recommended in JMeter as it blocks the execution thread, which can skew performance metrics and limit the scalability of the load test. Consider using a Flow Control Action sampler or a Timer for implementing backoff logic.
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
1 participant
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
sync=false) account-opening happy path that polls workflow state between transitions, with overridable base URL and load profile.get-instanceandget-instancestasks plus their old-key based mappings to enable querying existing workflow instances.initiate-account-openingschema that validates the start transition payload and minor cleanup inFunctionValidatePoliciesMapping.Changes
docker-compose.test.yml,jmeter/tests/workflow-test.jmx— JMeter test plan and isolated compose service joining the existingbbt-developmentnetworkREADME.md,.gitignore— Load testing usage docs and ignore forjmeter/results/core/Schemas/account-opening/initiate-account-opening.json— New start transition payload schemacore/Schemas/account-opening/account-type-selection.json— Allow extra properties for flexible client attributescore/Tasks/account-opening/get-instance.json,get-instances.json— New instance lookup taskscore/Workflows/account-opening/src/GetInstanceByOldKeyMapping.csx,GetInstancesByOldKeyMapping.csx— Mappings for the new taskscore/Workflows/account-opening/account-opening-workflow.json— Wire up the new tasks/schemacore/Functions/account-opening/multi-task-function-test.json,core/Functions/account-opening/src/FunctionValidatePoliciesMapping.csx— Remove stray double semicolonTest Plan
Made with Cursor
Summary by Sourcery
Add JMeter-based load testing for the asynchronous account-opening workflow and introduce workflow instance lookup capabilities keyed by legacy identifiers.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
Release Notes
New Features
Documentation