Add: behat test to test idempotentcy#94
Conversation
|
@johanib this ci still fails that is what we want. After merging OpenConext/Stepup-Middleware#595 and OpenConext/Stepup-Middleware#587 they should succeed. |
kayjoosten
force-pushed
the
feature/issue-587-behat-idempotent
branch
from
e312762 to
56dd92e
Compare
If applied, this commit will add an end-to-end Behat idempotency feature test and upgrade the Behat test container from PHP 7.2 to PHP 8.5. Why is this change needed? Prior to this change, the Behat container ran PHP 7.2, blocking modern PHP syntax in test code. There was also no automated test for the idempotency middleware behaviour. How does it address the issue? This change upgrades the Behat image to PHP 8.5 (node24), replaces abandoned mink-goutte-driver / mink-extension with mink-browserkit-driver and friends-of-behat/mink-extension, upgrades PHPUnit 5.7 → 12, and fixes MariaDB TLS compatibility (11.4+ requires TLS by default; disabled for the dev server). The CI job builds the image from the branch Dockerfile to avoid stale published images. A new 'I have the payload from file' step pushes the full middleware-config.json fixture so the idempotency test satisfies middleware's 'at least one service_provider' requirement.
kayjoosten
force-pushed
the
feature/issue-587-behat-idempotent
branch
from
c2fb05a to
fa6937d
Compare
| /** | ||
| * @Given I record the event stream count for aggregate :uuid | ||
| */ | ||
| public function iRecordEventStreamCount(string $uuid): void |
There was a problem hiding this comment.
These assertions are pretty specific for a high-over integration test. It tests specific behaviour for middleware.
Instead of 'I do X, I expect Y in the config.' this smells like a integration detail of middleware. Also, it requires db access to validate, amplifying the smell.
If we cannot test this from within Middleware, we can leave it in.
The USP for tests in devconf: They test multiple services or are impossible/hard to test in the project itself.
| */ | ||
| public function eventStreamCountShouldNotHaveIncreased(string $uuid): void | ||
| { | ||
| $result = shell_exec(sprintf( |
There was a problem hiding this comment.
Better to add
if (!preg_match('/^[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]{12}$/i', $uuid)) {
throw new \InvalidArgumentException(sprintf('Invalid UUID: "%s"', $uuid));
to prevent potential issues with shell exec.
| * @Given I record the event stream count for aggregate :uuid | ||
| */ | ||
| public function iRecordEventStreamCount(string $uuid): void | ||
| { |
There was a problem hiding this comment.
Better to add
if (!preg_match('/^[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]{12}$/i', $uuid)) {
throw new \InvalidArgumentException(sprintf('Invalid UUID: "%s"', $uuid));
to prevent potential issues with shell exec.
| When I request "POST /management/whitelist/replace" | ||
| Then the api response status code should be 200 | ||
| And the "status" property should equal "OK" | ||
| And I record the event stream count for aggregate "125ccee5-d650-437a-a0b0-6bf17c8188fa" |
There was a problem hiding this comment.
Scenario 2 title promises an assertion it never makes
"Pushing a changed whitelist does add an event" implies the test verifies the event count increases after the first (changed) push. It doesn't, it only records the count after step 1 and then verifies the second identical push is idempotent. A regression where even changed pushes stop creating events would silently pass this scenario.
Relevant lines: mw_idempotent_push.feature:47 (the second I record the event stream count step)
Suggested approach: Add a count-increase assertion between the first push and the second record step:
When I request "POST /management/whitelist/replace"
Then the api response status code should be 200
And the "status" property should equal "OK"
And the event stream count for aggregate "125ccee5-d650-437a-a0b0-6bf17c8188fa" should have increased
And I record the event stream count for aggregate "125ccee5-d650-437a-a0b0-6bf17c8188fa"
(This requires adding a complementary eventStreamCountShouldHaveIncreased step to FeatureContext.php.)
- Add UUID format validation before shell_exec in all event stream count step definitions to prevent potential injection - Add eventStreamCountShouldHaveIncreased step definition - Fix scenario 2: assert that a changed whitelist push does increase the event count before re-recording and verifying idempotency of the duplicate push
2 participants
No description provided.