Add configurable collect states to the simulated BankID environment - #553
Add configurable collect states to the simulated BankID environment#553torselden wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a way to configure the collect-state sequence used by the simulated BankID environment, enabling faster (and customizable) simulated authentication flows for automated testing scenarios.
Changes:
- Adds a
UseSimulatedEnvironment(Action<BankIdSimulatedEnvironmentOptions>)overload to configure collect-state sequences. - Introduces reusable collect-state presets (
NormalCollectStates,FastCollectStates,AllCollectStates) onBankIdSimulatedAppApiClient. - Adds tests and updates documentation describing how to use custom/preset collect-state sequences.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ActiveLogin.Authentication.BankId.Core.Test/BankIdBuilder_Tests.cs | Adds a test validating that configured collect states are used when registering the simulated environment. |
| test/ActiveLogin.Authentication.BankId.Api.Test/BankIdSimulatedAppApiClient_Tests.cs | Adds tests for custom collect states and the new “fast” preset. |
| src/ActiveLogin.Authentication.BankId.Core/IBankIdBuilderExtensions.cs | Adds an overload to configure simulated environment collect-state sequences. |
| src/ActiveLogin.Authentication.BankId.Core/BankIdSimulatedEnvironmentOptions.cs | Introduces options type holding the collect-state sequence configuration. |
| src/ActiveLogin.Authentication.BankId.AspNetCore/README.md | Documents using fast collect-state sequences for quicker automated tests. |
| src/ActiveLogin.Authentication.BankId.Api/BankIdSimulatedAppApiClient.cs | Adds collect-state preset properties and updates default constructor behavior. |
| docs/articles/bankid.md | Adds documentation/examples for configuring simulated environment collect states. |
Suppressed comments (1)
test/ActiveLogin.Authentication.BankId.Api.Test/BankIdSimulatedAppApiClient_Tests.cs:226
- Typo in test comment: "Arange" -> "Arrange".
// Arange
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/ActiveLogin.Authentication.BankId.Api.Test/BankIdSimulatedAppApiClient_Tests.cs:200
- Typo in test comment: "Arange" should be "Arrange".
// Arange
test/ActiveLogin.Authentication.BankId.Api.Test/BankIdSimulatedAppApiClient_Tests.cs:245
- Typo in test comment: "Arange" should be "Arrange".
// Arange
test/ActiveLogin.Authentication.BankId.Core.Test/BankIdBuilder_Tests.cs:37
- Test name contains a double underscore (
WithOptions__), which looks accidental and is inconsistent with the other test naming in this file. Consider renaming to a single underscore for readability.
public async Task UseSimulatedEnvironment_WithOptions__RegistersConfiguredCollectStates()
test/ActiveLogin.Authentication.BankId.Api.Test/BankIdSimulatedAppApiClient_Tests.cs:226
- Typo in test comment: "Arange" should be "Arrange".
This issue also appears on line 245 of the same file.
// Arange
test/ActiveLogin.Authentication.BankId.Core.Test/BankIdBuilder_Tests.cs:60
- This test verifies the second collect state's status but not its hint code. Since the configured state includes
CollectHintCode.UserSign, asserting it helps ensure the configured sequence is fully respected.
Assert.Equal(CollectStatus.Pending, firstCollectResponse.GetCollectStatus());
Assert.Equal(CollectHintCode.NoClient, firstCollectResponse.GetCollectHintCode());
Assert.Equal(CollectStatus.Complete, secondCollectResponse.GetCollectStatus());
}
| throw new ArgumentException("At least one collect state must be configured.", nameof(configure)); | ||
| } | ||
|
|
||
| var collectStates = new List<BankIdSimulatedAppApiClient.CollectState>(options.CollectStates); |
There was a problem hiding this comment.
Why do we copy the collect states both here and in the BankIdSimulatedAppApiClient?
There was a problem hiding this comment.
We copy the collect-state list to avoid shared mutable state. The configuration object and the simulated client both hold a list, and we want each layer to own its own snapshot. Otherwise, a caller can mutate the original list after construction and unexpectedly change the behavior of the simulated BankID flow. To defensive perhaps?
There was a problem hiding this comment.
A bit too defensive for my taste but it is fine :) One alterantive might be to take a IReadOnlyList as parameter instead.
There was a problem hiding this comment.
Removed builder-level defensive copy, kept client-level snapshot copy because that is the actual ownership boundary.
This PR fixes #397.
High level overview of this PR:
UseSimulatedEnvironmentto configure the collect-state sequence used during simulated authentication.Normal,Fast, andAllSuccessfulcollect-state sequences while preserving the existing default behavior.AllSuccessfulCollectStatescovers successful UI states and intentionally excludes failed terminal states.These things have been implemented: