You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "countradooku (via GitHub)" <gi...@apache.org> on 2026/06/03 08:37:18 UTC
[PR] test(php): add BDD suite wiring [iggy]
countradooku opened a new pull request, #3410:
URL: https://github.com/apache/iggy/pull/3410
## Summary
- add `bdd/php` with a PHPUnit implementation of the shared basic messaging BDD scenario
- wire `php-bdd` into the shared Docker Compose BDD runner and `scripts/run-bdd-tests.sh`
- add `bdd-php` component detection and include PHP BDD paths in `sdk-php`
Fixes #3310.
## Verification
- `php -l bdd/php/tests/BasicMessagingFeatureTest.php`
- parsed `bdd/php/phpunit.xml.dist` with PHP `SimpleXMLElement`
- `bash -n scripts/run-bdd-tests.sh`
- `git diff --check`
- `docker compose -f bdd/docker-compose.yml config`
- component YAML validation for `bdd-php` task/dependencies
- PHP BDD container against local Iggy server: `OK (1 test, 38 assertions)`
## Notes
A full local `./scripts/run-bdd-tests.sh php` run on macOS failed before PHP tests because the shared BDD server image copied local `target/debug` Mach-O binaries into a Linux container. CI builds Linux binaries before invoking the BDD runner, so the shared path should be valid there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(php): add BDD suite wiring [iggy]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3410:
URL: https://github.com/apache/iggy/pull/3410#issuecomment-4610631021
## [Codecov](https://app.codecov.io/gh/apache/iggy/pull/3410?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 73.38%. Comparing base ([`8ea5fee`](https://app.codecov.io/gh/apache/iggy/commit/8ea5feeb59e7250e024844cb324fc0e335c5aab3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a1b2a11`](https://app.codecov.io/gh/apache/iggy/commit/a1b2a11377f863af577f9782bedae7e9b1c112db?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3410 +/- ##
============================================
- Coverage 74.42% 73.38% -1.05%
Complexity 943 943
============================================
Files 1231 1231
Lines 120911 120911
Branches 97645 97645
============================================
- Hits 89994 88727 -1267
- Misses 27958 29316 +1358
+ Partials 2959 2868 -91
```
| [Components](https://app.codecov.io/gh/apache/iggy/pull/3410/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [Rust Core](https://app.codecov.io/gh/apache/iggy/pull/3410/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.58% <ø> (ø)` | |
| [Java SDK](https://app.codecov.io/gh/apache/iggy/pull/3410/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.44% <ø> (ø)` | |
| [C# SDK](https://app.codecov.io/gh/apache/iggy/pull/3410/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `69.96% <ø> (ø)` | |
| [Python SDK](https://app.codecov.io/gh/apache/iggy/pull/3410/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.06% <ø> (ø)` | |
| [Node SDK](https://app.codecov.io/gh/apache/iggy/pull/3410/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.32% <ø> (ø)` | |
| [Go SDK](https://app.codecov.io/gh/apache/iggy/pull/3410/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.32% <ø> (-25.89%)` | :arrow_down: |
[see 42 files with indirect coverage changes](https://app.codecov.io/gh/apache/iggy/pull/3410/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(php): add BDD suite wiring [iggy]
Posted by "countradooku (via GitHub)" <gi...@apache.org>.
countradooku commented on PR #3410:
URL: https://github.com/apache/iggy/pull/3410#issuecomment-4659866069
/ready
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(php): add BDD suite wiring [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio merged PR #3410:
URL: https://github.com/apache/iggy/pull/3410
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(php): add BDD suite wiring [iggy]
Posted by "countradooku (via GitHub)" <gi...@apache.org>.
countradooku commented on PR #3410:
URL: https://github.com/apache/iggy/pull/3410#issuecomment-4668234089
/ready
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(php): add BDD suite wiring [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #3410:
URL: https://github.com/apache/iggy/pull/3410#discussion_r3394449130
##########
scripts/ci/sync-rustc-version.sh:
##########
@@ -101,7 +101,7 @@ for dockerfile in $DOCKERFILES; do
EXPECTED_VERSION="$RUST_VERSION_SHORT"
elif grep -qE "FROM[[:space:]].*\brust:[0-9]" "$dockerfile" 2>/dev/null; then
SOURCE="from"
- CURRENT_VERSION=$(grep -E "FROM[[:space:]].*\brust:[0-9]" "$dockerfile" | head -1 | sed -nE 's/.*\brust:([0-9]+\.[0-9]+(\.[0-9]+)?).*/\1/p')
+ CURRENT_VERSION=$(grep -E "FROM[[:space:]].*\brust:[0-9]" "$dockerfile" | head -1 | sed -nE 's/.*rust:([0-9]+\.[0-9]+(\.[0-9]+)?).*/\1/p')
Review Comment:
also at line 129.
why drop `\b` here? the php Dockerfile uses the `ARG RUST_VERSION` form, so neither of these sed lines ever runs for it - this change is unrelated to the PR. it also loosens fix mode: `s/(rust:)[0-9]+.../g` now rewrites any image name that merely ends in `rust:`. revert, or if there's a concrete reason (BSD sed on macOS not supporting `\b`?), say so in the commit.
##########
bdd/php/Dockerfile:
##########
@@ -0,0 +1,81 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+ARG RUST_VERSION=1.96
+FROM rust:${RUST_VERSION}-slim-trixie AS rust-toolchain
+
+FROM ubuntu:24.04
+
+COPY --from=rust-toolchain /usr/local/cargo /usr/local/cargo
+COPY --from=rust-toolchain /usr/local/rustup /usr/local/rustup
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ build-essential \
+ ca-certificates \
+ clang \
+ composer \
+ curl \
+ git \
+ libclang-dev \
+ libssl-dev \
+ php8.3-cli \
+ php8.3-dev \
+ php8.3-mbstring \
+ php8.3-xml \
+ pkg-config \
+ unzip \
+ && rm -rf /var/lib/apt/lists/*
+
+ENV CARGO_HOME=/usr/local/cargo
+ENV RUSTUP_HOME=/usr/local/rustup
+ENV PATH=/usr/local/cargo/bin:$PATH
+ENV PHP=/usr/bin/php8.3
+ENV PHP_CONFIG=/usr/bin/php-config8.3
+ENV IGGY_HOST=iggy-server
+ENV IGGY_PORT=8090
+ENV BDD_FEATURE_FILE=/app/features/basic_messaging.feature
Review Comment:
this `ENV` still points at `/app/features/...`, which only exists via the compose mount - so a bare `docker run` of this image throws `feature file not found` even though the scenarios are now baked in at `/workspace/bdd/scenarios`. the env override preempts the in-test fallback path. point it at `/workspace/bdd/scenarios/basic_messaging.feature` (compose already sets its own value) or drop the `ENV` entirely and let the fallback resolve.
##########
bdd/php/tests/BasicMessagingFeatureTest.php:
##########
@@ -0,0 +1,326 @@
+<?php
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+declare(strict_types=1);
+
+use Iggy\Client as IggyClient;
+use Iggy\PollingStrategy;
+use Iggy\SendMessage;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\TestDox;
+use PHPUnit\Framework\TestCase;
+
+final class BasicMessagingFeatureTest extends TestCase
+{
+ private ?IggyClient $client = null;
+ private ?int $lastStreamId = null;
+ private ?string $lastStreamName = null;
+ private ?string $lastTopicName = null;
+ private array $lastPolledMessages = [];
+ private array $sentPayloads = [];
+ private ?int $lastSentMessageCount = null;
+
+ #[DataProvider('scenarioCases')]
+ #[TestDox('Basic messaging shared BDD scenario passes for the PHP SDK')]
+ public function testBasicMessagingScenario(string $scenarioName, array $steps): void
+ {
+ assert_true($scenarioName !== '', 'scenario name must not be empty');
+
+ foreach ($steps as $step) {
+ $this->runStep($step);
+ }
+ }
+
+ protected function tearDown(): void
+ {
+ if ($this->client !== null && $this->lastStreamName !== null) {
+ cleanup_stream_with_topics(
+ $this->client,
+ $this->lastStreamName,
+ $this->lastTopicName !== null ? [$this->lastTopicName] : [],
+ );
+ }
+
+ parent::tearDown();
+ }
+
+ public static function scenarioCases(): array
+ {
+ $featureFile = getenv('BDD_FEATURE_FILE') ?: __DIR__ . '/../../scenarios/basic_messaging.feature';
+ if (!is_file($featureFile)) {
+ throw new RuntimeException("feature file not found at {$featureFile}");
+ }
+
+ $lines = file($featureFile, FILE_IGNORE_NEW_LINES);
+ if ($lines === false) {
+ throw new RuntimeException("failed to read feature file at {$featureFile}");
+ }
+
+ $backgroundSteps = [];
+ $scenarios = [];
+ $currentScenario = null;
+ $currentSteps = [];
+ $section = null;
+ $stepPattern = '/^(Given|When|Then|And|But|\*) (.+)$/';
+ foreach ($lines as $line) {
+ $line = trim($line);
+ if ($line === '' || str_starts_with($line, '#') || str_starts_with($line, '@')) {
+ continue;
+ }
+
+ if ($line === 'Background:') {
+ $section = 'background';
+ continue;
+ }
+
+ if (str_starts_with($line, 'Scenario:')) {
+ if ($currentScenario !== null) {
+ $scenarios[$currentScenario] = $currentSteps;
+ }
+
+ $currentScenario = trim(substr($line, strlen('Scenario:')));
+ $currentSteps = [];
+ $section = 'scenario';
+ continue;
+ }
+
+ if (preg_match($stepPattern, $line, $matches) !== 1) {
Review Comment:
structural gherkin keywords fall through this regex silently - `Scenario Outline:`, `Rule:`, `Examples:` and `|` table rows all get dropped, and outline steps would attach to the previous scenario since `section` stays `'scenario'`. the shared feature file is meant to evolve across sdks, so failing fast on lines that start with these known structural keywords is cheap and keeps this honest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(php): add BDD suite wiring [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #3410:
URL: https://github.com/apache/iggy/pull/3410#discussion_r3381627661
##########
bdd/php/tests/BasicMessagingFeatureTest.php:
##########
@@ -0,0 +1,324 @@
+<?php
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+declare(strict_types=1);
+
+use Iggy\Client as IggyClient;
+use Iggy\PollingStrategy;
+use Iggy\SendMessage;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\TestDox;
+use PHPUnit\Framework\TestCase;
+
+final class BasicMessagingFeatureTest extends TestCase
+{
+ private ?IggyClient $client = null;
+ private ?int $lastStreamId = null;
+ private ?string $lastStreamName = null;
+ private ?string $lastTopicName = null;
+ private ?int $lastTopicPartitions = null;
+ private array $lastPolledMessages = [];
+ private array $sentPayloads = [];
+ private ?int $lastSentMessageCount = null;
+
+ #[DataProvider('scenarioCases')]
+ #[TestDox('Basic messaging shared BDD scenario passes for the PHP SDK')]
+ public function testBasicMessagingScenario(string $scenarioName, array $steps): void
+ {
+ assert_true($scenarioName !== '', 'scenario name must not be empty');
+
+ foreach ($steps as $step) {
+ $this->runStep($step);
+ }
+ }
+
+ protected function tearDown(): void
+ {
+ if ($this->client !== null && $this->lastStreamName !== null) {
+ cleanup_stream_with_topics(
+ $this->client,
+ $this->lastStreamName,
+ $this->lastTopicName !== null ? [$this->lastTopicName] : [],
+ );
+ }
+
+ parent::tearDown();
+ }
+
+ public static function scenarioCases(): array
+ {
+ $featureFile = getenv('BDD_FEATURE_FILE') ?: __DIR__ . '/../../scenarios/basic_messaging.feature';
+ if (!is_file($featureFile)) {
+ throw new RuntimeException("feature file not found at {$featureFile}");
+ }
+
+ $lines = file($featureFile, FILE_IGNORE_NEW_LINES);
+ if ($lines === false) {
+ throw new RuntimeException("failed to read feature file at {$featureFile}");
+ }
+
+ $backgroundSteps = [];
+ $scenarios = [];
+ $currentScenario = null;
+ $currentSteps = [];
+ $section = null;
+ foreach ($lines as $line) {
+ $line = trim($line);
+ if ($line === '' || str_starts_with($line, '#') || str_starts_with($line, '@')) {
+ continue;
+ }
+
+ if ($line === 'Background:') {
+ $section = 'background';
+ continue;
+ }
+
+ if (str_starts_with($line, 'Scenario:')) {
+ if ($currentScenario !== null) {
+ $scenarios[$currentScenario] = $currentSteps;
+ }
+
+ $currentScenario = trim(substr($line, strlen('Scenario:')));
+ $currentSteps = [];
+ $section = 'scenario';
+ continue;
+ }
+
+ if (preg_match('/^(Given|When|Then|And|But|\*) (.+)$/', $line, $matches) !== 1) {
+ continue;
+ }
+
+ if ($section === 'background') {
+ $backgroundSteps[] = $matches[2];
+ continue;
+ }
+
+ if ($section === 'scenario' && $currentScenario !== null) {
+ $currentSteps[] = $matches[2];
+ }
+ }
+
+ if ($currentScenario !== null) {
+ $scenarios[$currentScenario] = $currentSteps;
+ }
+
+ if ($backgroundSteps === []) {
+ throw new RuntimeException('no BDD background steps were loaded from the feature');
+ }
+
+ if ($scenarios === []) {
+ throw new RuntimeException('no BDD scenarios were loaded from the feature');
+ }
+
+ $cases = [];
+ foreach ($scenarios as $scenarioName => $scenarioSteps) {
+ if ($scenarioSteps === []) {
+ throw new RuntimeException("scenario has no steps: {$scenarioName}");
+ }
+
+ $cases[$scenarioName] = [$scenarioName, [...$backgroundSteps, ...$scenarioSteps]];
+ }
+
+ return $cases;
+ }
+
+ private function runStep(string $step): void
+ {
+ if ($step === 'I have a running Iggy server') {
+ $this->client = new IggyClient(server_host() . ':' . server_port());
+ $this->client->connect();
+ $this->client->ping();
+
+ return;
+ }
+
+ if ($step === 'I am authenticated as the root user') {
+ $this->requireClient()->loginUser(env_or_default('IGGY_USERNAME', 'iggy'), env_or_default('IGGY_PASSWORD', 'iggy'));
+
+ return;
+ }
+
+ if ($step === 'I have no streams in the system') {
+ return;
+ }
+
+ if (preg_match('/^I create a stream with name "([^"]+)"$/', $step, $matches) === 1) {
+ $streamName = $matches[1];
+ $this->requireClient()->createStream($streamName);
+ $stream = $this->requireClient()->getStream($streamName);
+ assert_not_null($stream);
+ $this->lastStreamId = $stream->id;
+ $this->lastStreamName = $streamName;
+
+ return;
+ }
+
+ if ($step === 'the stream should be created successfully') {
+ assert_not_null($this->requireClient()->getStream($this->requireStreamName()));
+
+ return;
+ }
+
+ if (preg_match('/^the stream should have name "([^"]+)"$/', $step, $matches) === 1) {
+ $stream = $this->requireClient()->getStream($matches[1]);
+ assert_not_null($stream);
+ assert_same($matches[1], $stream->name);
+
+ return;
+ }
+
+ if (preg_match('/^I create a topic with name "([^"]+)" in stream (\d+) with (\d+) partitions$/', $step, $matches) === 1) {
+ $topicName = $matches[1];
+ $streamId = (int) $matches[2];
+ $partitions = (int) $matches[3];
+
+ $this->requireClient()->createTopic($streamId, $topicName, $partitions, null, null, null, null);
+ $topic = $this->requireClient()->getTopic($streamId, $topicName);
+ assert_not_null($topic);
+ $this->lastTopicName = $topicName;
+ $this->lastTopicPartitions = $partitions;
+
+ return;
+ }
+
+ if ($step === 'the topic should be created successfully') {
+ assert_not_null($this->requireClient()->getTopic($this->requireStreamId(), $this->requireTopicName()));
+
+ return;
+ }
+
+ if (preg_match('/^the topic should have name "([^"]+)"$/', $step, $matches) === 1) {
+ $topic = $this->requireClient()->getTopic($this->requireStreamId(), $matches[1]);
+ assert_not_null($topic);
+ assert_same($matches[1], $topic->name);
+
+ return;
+ }
+
+ if (preg_match('/^the topic should have (\d+) partitions$/', $step, $matches) === 1) {
+ $topic = $this->requireClient()->getTopic($this->requireStreamId(), $this->requireTopicName());
+ assert_not_null($topic);
+ assert_same((int) $matches[1], $topic->partitions_count);
+ assert_same((int) $matches[1], $this->lastTopicPartitions);
Review Comment:
this assert is redundant - line 216 already checks `topic->partitions_count` from the server response against the expected count, so re-checking the locally-captured `lastTopicPartitions` against the same value adds nothing. you can drop both this line and the `lastTopicPartitions` field.
##########
bdd/php/Dockerfile:
##########
@@ -0,0 +1,74 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+FROM ubuntu:24.04
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ build-essential \
+ ca-certificates \
+ clang \
+ composer \
+ curl \
+ git \
+ libclang-dev \
+ libssl-dev \
+ php8.3-cli \
+ php8.3-dev \
+ php8.3-mbstring \
+ php8.3-xml \
+ pkg-config \
+ unzip \
+ && rm -rf /var/lib/apt/lists/*
+
+ENV PATH=/root/.cargo/bin:$PATH
+ENV PHP=/usr/bin/php8.3
+ENV PHP_CONFIG=/usr/bin/php-config8.3
+ENV IGGY_HOST=iggy-server
+ENV IGGY_PORT=8090
+ENV BDD_FEATURE_FILE=/app/features/basic_messaging.feature
+
+RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs \
+ | sh -s -- -y --profile minimal --default-toolchain 1.95.0 \
Review Comment:
the `--default-toolchain 1.95.0` rustup form here doesn't match the rest of the repo. master pins 1.96.0 in `rust-toolchain.toml`, and since the COPY block below never copies that file, this 1.95 is what actually builds the ext and the sdk crates. the other bdd images use `ARG RUST_VERSION` + `FROM rust:${RUST_VERSION}`, and `scripts/ci/sync-rustc-version.sh` only knows how to rewrite those two forms - it skips the rustup line entirely, so this pin is invisible to the version-sync gate and will silently drift on the next toolchain bump. switch to the `ARG RUST_VERSION` + `FROM rust:` shape the siblings use.
##########
bdd/docker-compose.coverage.yml:
##########
@@ -45,6 +45,24 @@ services:
pytest tests/ -v
--cov --cov-report=xml:/reports/python-bdd-coverage.xml
+ php-bdd:
+ volumes:
+ - ../reports:/reports
+ command:
+ - bash
+ - -lc
+ - >-
+ cargo install cargo-llvm-cov --locked
Review Comment:
this coverage path can't actually produce a report as-is. the image installs rust with `--profile minimal` (Dockerfile:45) and never adds `llvm-tools-preview`, but `cargo llvm-cov` needs it - rust-bdd adds the component explicitly and php doesn't, so `cargo llvm-cov show-env` and `report` here will fail. separately, `cargo install cargo-llvm-cov` runs at container runtime over the network on every coverage run - bake it into the image instead.
##########
bdd/php/Dockerfile:
##########
@@ -0,0 +1,74 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+FROM ubuntu:24.04
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ build-essential \
+ ca-certificates \
+ clang \
+ composer \
+ curl \
+ git \
+ libclang-dev \
+ libssl-dev \
+ php8.3-cli \
+ php8.3-dev \
+ php8.3-mbstring \
+ php8.3-xml \
+ pkg-config \
+ unzip \
+ && rm -rf /var/lib/apt/lists/*
+
+ENV PATH=/root/.cargo/bin:$PATH
+ENV PHP=/usr/bin/php8.3
+ENV PHP_CONFIG=/usr/bin/php-config8.3
+ENV IGGY_HOST=iggy-server
+ENV IGGY_PORT=8090
+ENV BDD_FEATURE_FILE=/app/features/basic_messaging.feature
+
+RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs \
+ | sh -s -- -y --profile minimal --default-toolchain 1.95.0 \
+ && cargo install cargo-php --locked --version 0.1.21 \
+ && rm -rf /root/.cargo/registry /root/.cargo/git
+
+WORKDIR /workspace
+
+COPY Cargo.toml Cargo.lock ./
+COPY core/binary_protocol/ ./core/binary_protocol/
+COPY core/common/ ./core/common/
+COPY core/sdk/ ./core/sdk/
+
+COPY foreign/php/Cargo.toml ./foreign/php/
+COPY foreign/php/composer.json ./foreign/php/
+COPY foreign/php/phpunit.xml.dist ./foreign/php/
+COPY foreign/php/README.md foreign/php/LICENSE foreign/php/NOTICE ./foreign/php/
+COPY foreign/php/.cargo/ ./foreign/php/.cargo/
+COPY foreign/php/src/ ./foreign/php/src/
+COPY foreign/php/tests/ ./foreign/php/tests/
+COPY foreign/php/scripts/ ./foreign/php/scripts/
+
+COPY bdd/php/ ./bdd/php/
+COPY bdd/scenarios/ ./bdd/scenarios/
+
+WORKDIR /workspace/foreign/php
+
+RUN composer install --no-interaction --prefer-dist
+RUN cargo php install --yes
Review Comment:
`cargo php install` recompiles the sdk closure (binary_protocol + common + sdk) on every `up --build`, and line 47 wipes the cargo registry cache so there's no dep cache to reuse across builds. a cache mount here would avoid the repeated cold rebuild. test-only so not urgent.
##########
bdd/php/tests/BasicMessagingFeatureTest.php:
##########
@@ -0,0 +1,324 @@
+<?php
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+declare(strict_types=1);
+
+use Iggy\Client as IggyClient;
+use Iggy\PollingStrategy;
+use Iggy\SendMessage;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\TestDox;
+use PHPUnit\Framework\TestCase;
+
+final class BasicMessagingFeatureTest extends TestCase
+{
+ private ?IggyClient $client = null;
+ private ?int $lastStreamId = null;
+ private ?string $lastStreamName = null;
+ private ?string $lastTopicName = null;
+ private ?int $lastTopicPartitions = null;
+ private array $lastPolledMessages = [];
+ private array $sentPayloads = [];
+ private ?int $lastSentMessageCount = null;
+
+ #[DataProvider('scenarioCases')]
+ #[TestDox('Basic messaging shared BDD scenario passes for the PHP SDK')]
+ public function testBasicMessagingScenario(string $scenarioName, array $steps): void
+ {
+ assert_true($scenarioName !== '', 'scenario name must not be empty');
+
+ foreach ($steps as $step) {
+ $this->runStep($step);
+ }
+ }
+
+ protected function tearDown(): void
+ {
+ if ($this->client !== null && $this->lastStreamName !== null) {
+ cleanup_stream_with_topics(
+ $this->client,
+ $this->lastStreamName,
+ $this->lastTopicName !== null ? [$this->lastTopicName] : [],
+ );
+ }
+
+ parent::tearDown();
+ }
+
+ public static function scenarioCases(): array
+ {
+ $featureFile = getenv('BDD_FEATURE_FILE') ?: __DIR__ . '/../../scenarios/basic_messaging.feature';
+ if (!is_file($featureFile)) {
+ throw new RuntimeException("feature file not found at {$featureFile}");
+ }
+
+ $lines = file($featureFile, FILE_IGNORE_NEW_LINES);
+ if ($lines === false) {
+ throw new RuntimeException("failed to read feature file at {$featureFile}");
+ }
+
+ $backgroundSteps = [];
+ $scenarios = [];
+ $currentScenario = null;
+ $currentSteps = [];
+ $section = null;
+ foreach ($lines as $line) {
+ $line = trim($line);
+ if ($line === '' || str_starts_with($line, '#') || str_starts_with($line, '@')) {
+ continue;
+ }
+
+ if ($line === 'Background:') {
+ $section = 'background';
+ continue;
+ }
+
+ if (str_starts_with($line, 'Scenario:')) {
+ if ($currentScenario !== null) {
+ $scenarios[$currentScenario] = $currentSteps;
+ }
+
+ $currentScenario = trim(substr($line, strlen('Scenario:')));
+ $currentSteps = [];
+ $section = 'scenario';
+ continue;
+ }
+
+ if (preg_match('/^(Given|When|Then|And|But|\*) (.+)$/', $line, $matches) !== 1) {
Review Comment:
step lines that appear before the first `Background:` or `Scenario:` get silently dropped here - `section` is still null so neither branch appends them, and they never reach the `self::fail` for unknown steps. the feature has none today, but a misplaced step in the shared file would vanish instead of failing. cheap to guard: fail when `section` is null and the line matches the step regex.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(php): add BDD suite wiring [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3410:
URL: https://github.com/apache/iggy/pull/3410#issuecomment-4610530441
<!-- iggy-pr-triage-welcome -->
Thanks for the PR. It is labeled `S-waiting-on-review` and queued for review.
Slash commands (own line, regular comment) move it around the queue:
- `/ready` - back to `S-waiting-on-review` after addressing feedback
- `/author` - flip to `S-waiting-on-author` while you finish changes
- `/request-review @user-or-team` - request a reviewer
See [CONTRIBUTING.md](https://github.com/apache/iggy/blob/master/CONTRIBUTING.md#pr-triage-commands) for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(php): add BDD suite wiring [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #3410:
URL: https://github.com/apache/iggy/pull/3410#discussion_r3378639378
##########
licenserc.toml:
##########
@@ -76,6 +76,7 @@ excludes = [
"**/.env",
"**/.gitkeep",
"**/META-INF/services/**",
+ "bdd/php/tests/BasicMessagingFeatureTest.php",
Review Comment:
this excludes the test from license-header enforcement permanently. root cause is header style: `licenserc.toml` maps `php` to `DOUBLESLASH_STYLE` (line 88) so hawkeye wants `//` headers, but this file uses a `/* */` block header. every other tracked `.php` (e.g. `foreign/php/tests/bootstrap.php`) uses `//`. instead of disabling the gate on a brand-new source file, switch the header to `//` and drop this exclude - hawkeye passes that way.
##########
.github/actions/php/pre-merge/action.yml:
##########
@@ -58,7 +58,7 @@ runs:
- name: Setup Rust with cache
uses: ./.github/actions/utils/setup-rust-with-cache
with:
- shared-key: dev
+ shared-key: php
Review Comment:
this switches the rust cache `shared-key` from `dev` to `php`, but the job is read-only (`save-cache: false` right below) and nothing in the repo writes a `php` key - the only cache writers live in `coverage-baseline.yml` and they save under the default `dev`. net effect: the php pre-merge job now misses the rust cache every run and does a full cold build, where `dev` used to be warm.
we dont need php cache, its fast enough. drop this change.
##########
bdd/php/Dockerfile:
##########
@@ -0,0 +1,66 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+FROM rust:1.95-slim-trixie
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ ca-certificates \
+ clang \
+ composer \
+ curl \
+ git \
+ libclang-dev \
+ libssl-dev \
+ php-cli \
+ php-dev \
+ php-mbstring \
+ php-xml \
+ pkg-config \
+ unzip \
+ && rm -rf /var/lib/apt/lists/*
+
+ENV PHP=/usr/bin/php
+ENV PHP_CONFIG=/usr/bin/php-config
+ENV IGGY_HOST=iggy-server
+ENV IGGY_PORT=8090
+ENV BDD_FEATURE_FILE=/app/features/basic_messaging.feature
+
+WORKDIR /workspace
+
+COPY Cargo.toml Cargo.lock ./
+COPY core/ ./core/
+
+COPY foreign/php/Cargo.toml ./foreign/php/
+COPY foreign/php/composer.json ./foreign/php/
+COPY foreign/php/phpunit.xml.dist ./foreign/php/
+COPY foreign/php/README.md foreign/php/LICENSE foreign/php/NOTICE ./foreign/php/
+COPY foreign/php/.cargo/ ./foreign/php/.cargo/
+COPY foreign/php/src/ ./foreign/php/src/
+COPY foreign/php/tests/ ./foreign/php/tests/
+COPY foreign/php/scripts/ ./foreign/php/scripts/
+
+COPY bdd/php/ ./bdd/php/
+
+WORKDIR /workspace/foreign/php
+
+RUN cargo install cargo-php --locked --version 0.1.21 \
Review Comment:
`cargo install cargo-php` sits after all the `COPY` layers, but it has no dependency on the copied workspace. any source edit busts the cache here and rebuilds cargo-php from source on every local/cached build. move this `RUN` above the `COPY` lines (right after the apt-get) so it caches independently.
##########
bdd/php/Dockerfile:
##########
@@ -0,0 +1,66 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+FROM rust:1.95-slim-trixie
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ ca-certificates \
+ clang \
+ composer \
+ curl \
+ git \
+ libclang-dev \
+ libssl-dev \
+ php-cli \
+ php-dev \
+ php-mbstring \
+ php-xml \
+ pkg-config \
+ unzip \
+ && rm -rf /var/lib/apt/lists/*
+
+ENV PHP=/usr/bin/php
+ENV PHP_CONFIG=/usr/bin/php-config
+ENV IGGY_HOST=iggy-server
+ENV IGGY_PORT=8090
+ENV BDD_FEATURE_FILE=/app/features/basic_messaging.feature
+
+WORKDIR /workspace
+
+COPY Cargo.toml Cargo.lock ./
+COPY core/ ./core/
+
+COPY foreign/php/Cargo.toml ./foreign/php/
+COPY foreign/php/composer.json ./foreign/php/
+COPY foreign/php/phpunit.xml.dist ./foreign/php/
+COPY foreign/php/README.md foreign/php/LICENSE foreign/php/NOTICE ./foreign/php/
+COPY foreign/php/.cargo/ ./foreign/php/.cargo/
+COPY foreign/php/src/ ./foreign/php/src/
+COPY foreign/php/tests/ ./foreign/php/tests/
+COPY foreign/php/scripts/ ./foreign/php/scripts/
+
+COPY bdd/php/ ./bdd/php/
Review Comment:
the image never copies `bdd/scenarios`, so the feature file only exists via the compose volume mount. the `BDD_FEATURE_FILE` default and the in-test fallback path both point at locations that don't exist inside the image, so a bare `docker run` of this image can't find the feature. every other bdd image bakes the scenarios in (rust/python/java via `COPY . .`, go/node/csharp explicitly). add `COPY bdd/scenarios ./bdd/scenarios` for parity.
##########
bdd/php/tests/BasicMessagingFeatureTest.php:
##########
@@ -0,0 +1,278 @@
+<?php
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+declare(strict_types=1);
+
+use Iggy\Client as IggyClient;
+use Iggy\PollingStrategy;
+use Iggy\SendMessage;
+use PHPUnit\Framework\Attributes\TestDox;
+use PHPUnit\Framework\TestCase;
+
+final class BasicMessagingFeatureTest extends TestCase
+{
+ private ?IggyClient $client = null;
+ private ?int $lastStreamId = null;
+ private ?string $lastStreamName = null;
+ private ?string $lastTopicName = null;
+ private ?int $lastTopicPartitions = null;
+ private array $lastPolledMessages = [];
+ private array $sentPayloads = [];
+ private ?int $lastSentMessageCount = null;
+
+ #[TestDox('Basic messaging shared BDD scenario passes for the PHP SDK')]
+ public function testBasicMessagingScenario(): void
+ {
+ foreach ($this->scenarioSteps() as $step) {
+ $this->runStep($step);
+ }
+ }
+
+ protected function tearDown(): void
+ {
+ if ($this->client !== null && $this->lastStreamName !== null) {
+ cleanup_stream_with_topics(
+ $this->client,
+ $this->lastStreamName,
+ $this->lastTopicName !== null ? [$this->lastTopicName] : [],
+ );
+ }
+
+ parent::tearDown();
+ }
+
+ private function scenarioSteps(): array
Review Comment:
this hand-rolls a gherkin parser plus a big `if`/`preg_match` dispatch chain. every other sdk uses a real cucumber binding (cucumber-rs, pytest-bdd, godog, cucumber-js, cucumber-jvm, Reqnroll); the php equivalent is Behat and it isn't pulled in. the runtime parse here is also mostly redundant - `runStep` already hardcodes every step regex, and the scenario title + feature path are hardcoded, so the parse buys very little. adopting Behat (or at least driving off the parsed steps properly) would replace this whole block and fix the silent-skip issues flagged below at once.
##########
bdd/php/Dockerfile:
##########
@@ -0,0 +1,66 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+FROM rust:1.95-slim-trixie
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ ca-certificates \
+ clang \
+ composer \
+ curl \
+ git \
+ libclang-dev \
+ libssl-dev \
+ php-cli \
Review Comment:
php is unpinned here, so on `trixie` you get php 8.4 while the ci action pins `php8.3`. the bdd run and the sdk tests end up on different php minors, which can drift on a base-image bump. pin `php8.3-*` to match ci.
##########
bdd/php/tests/BasicMessagingFeatureTest.php:
##########
@@ -0,0 +1,278 @@
+<?php
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+declare(strict_types=1);
+
+use Iggy\Client as IggyClient;
+use Iggy\PollingStrategy;
+use Iggy\SendMessage;
+use PHPUnit\Framework\Attributes\TestDox;
+use PHPUnit\Framework\TestCase;
+
+final class BasicMessagingFeatureTest extends TestCase
+{
+ private ?IggyClient $client = null;
+ private ?int $lastStreamId = null;
+ private ?string $lastStreamName = null;
+ private ?string $lastTopicName = null;
+ private ?int $lastTopicPartitions = null;
+ private array $lastPolledMessages = [];
+ private array $sentPayloads = [];
+ private ?int $lastSentMessageCount = null;
+
+ #[TestDox('Basic messaging shared BDD scenario passes for the PHP SDK')]
+ public function testBasicMessagingScenario(): void
+ {
+ foreach ($this->scenarioSteps() as $step) {
+ $this->runStep($step);
+ }
+ }
+
+ protected function tearDown(): void
+ {
+ if ($this->client !== null && $this->lastStreamName !== null) {
+ cleanup_stream_with_topics(
+ $this->client,
+ $this->lastStreamName,
+ $this->lastTopicName !== null ? [$this->lastTopicName] : [],
+ );
+ }
+
+ parent::tearDown();
+ }
+
+ private function scenarioSteps(): array
+ {
+ $featureFile = getenv('BDD_FEATURE_FILE') ?: __DIR__ . '/../../scenarios/basic_messaging.feature';
+ assert_true(is_file($featureFile), "feature file not found at {$featureFile}");
+
+ $lines = file($featureFile, FILE_IGNORE_NEW_LINES);
+ assert_true($lines !== false, "failed to read feature file at {$featureFile}");
+
+ $steps = [];
+ $insideTargetScenario = false;
+ foreach ($lines as $line) {
+ $line = trim($line);
+ if ($line === '') {
+ continue;
+ }
+
+ if (str_starts_with($line, 'Scenario:')) {
+ $insideTargetScenario = $line === 'Scenario: Create stream and send messages';
+ continue;
+ }
+
+ if (!$insideTargetScenario) {
+ continue;
+ }
+
+ if (preg_match('/^(Given|When|Then|And) (.+)$/', $line, $matches) === 1) {
Review Comment:
this regex only matches `Given|When|Then|And`, but gherkin also allows `But` (and `*`). such a step returns `preg_match` 0 and gets dropped during parsing, so it never reaches the `self::fail` for unhandled steps - it just silently disappears. no `But` in the feature today, but worth widening the alternation.
##########
bdd/php/tests/BasicMessagingFeatureTest.php:
##########
@@ -0,0 +1,278 @@
+<?php
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+declare(strict_types=1);
+
+use Iggy\Client as IggyClient;
+use Iggy\PollingStrategy;
+use Iggy\SendMessage;
+use PHPUnit\Framework\Attributes\TestDox;
+use PHPUnit\Framework\TestCase;
+
+final class BasicMessagingFeatureTest extends TestCase
+{
+ private ?IggyClient $client = null;
+ private ?int $lastStreamId = null;
+ private ?string $lastStreamName = null;
+ private ?string $lastTopicName = null;
+ private ?int $lastTopicPartitions = null;
+ private array $lastPolledMessages = [];
+ private array $sentPayloads = [];
+ private ?int $lastSentMessageCount = null;
+
+ #[TestDox('Basic messaging shared BDD scenario passes for the PHP SDK')]
+ public function testBasicMessagingScenario(): void
+ {
+ foreach ($this->scenarioSteps() as $step) {
+ $this->runStep($step);
+ }
+ }
+
+ protected function tearDown(): void
+ {
+ if ($this->client !== null && $this->lastStreamName !== null) {
+ cleanup_stream_with_topics(
+ $this->client,
+ $this->lastStreamName,
+ $this->lastTopicName !== null ? [$this->lastTopicName] : [],
+ );
+ }
+
+ parent::tearDown();
+ }
+
+ private function scenarioSteps(): array
+ {
+ $featureFile = getenv('BDD_FEATURE_FILE') ?: __DIR__ . '/../../scenarios/basic_messaging.feature';
+ assert_true(is_file($featureFile), "feature file not found at {$featureFile}");
+
+ $lines = file($featureFile, FILE_IGNORE_NEW_LINES);
+ assert_true($lines !== false, "failed to read feature file at {$featureFile}");
+
+ $steps = [];
+ $insideTargetScenario = false;
+ foreach ($lines as $line) {
+ $line = trim($line);
+ if ($line === '') {
+ continue;
+ }
+
+ if (str_starts_with($line, 'Scenario:')) {
+ $insideTargetScenario = $line === 'Scenario: Create stream and send messages';
Review Comment:
the parser only collects steps when the scenario title is exactly `Create stream and send messages`, and nothing asserts how many scenarios ran. the moment a second scenario is added to the shared feature, php silently skips it while rust (`World::run`) and python (`scenarios()`) run all of them. silent false-green against a feature file that's meant to be shared across sdks.
##########
bdd/docker-compose.yml:
##########
@@ -221,6 +221,25 @@ services:
networks:
- iggy-bdd-network
+ php-bdd:
Review Comment:
there's no `php-bdd` entry in `docker-compose.coverage.yml`, so `run-bdd-tests.sh --coverage php` (and the `all` path) runs this uninstrumented service and silently produces no coverage report. every other sdk has a coverage override.
add one for php or note that php bdd coverage is intentionally skipped. (and create gh issue for this)
or implement it now.
##########
bdd/php/tests/BasicMessagingFeatureTest.php:
##########
@@ -0,0 +1,278 @@
+<?php
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+declare(strict_types=1);
+
+use Iggy\Client as IggyClient;
+use Iggy\PollingStrategy;
+use Iggy\SendMessage;
+use PHPUnit\Framework\Attributes\TestDox;
+use PHPUnit\Framework\TestCase;
+
+final class BasicMessagingFeatureTest extends TestCase
+{
+ private ?IggyClient $client = null;
+ private ?int $lastStreamId = null;
+ private ?string $lastStreamName = null;
+ private ?string $lastTopicName = null;
+ private ?int $lastTopicPartitions = null;
+ private array $lastPolledMessages = [];
+ private array $sentPayloads = [];
+ private ?int $lastSentMessageCount = null;
+
+ #[TestDox('Basic messaging shared BDD scenario passes for the PHP SDK')]
+ public function testBasicMessagingScenario(): void
+ {
+ foreach ($this->scenarioSteps() as $step) {
+ $this->runStep($step);
+ }
+ }
+
+ protected function tearDown(): void
+ {
+ if ($this->client !== null && $this->lastStreamName !== null) {
+ cleanup_stream_with_topics(
+ $this->client,
+ $this->lastStreamName,
+ $this->lastTopicName !== null ? [$this->lastTopicName] : [],
+ );
+ }
+
+ parent::tearDown();
+ }
+
+ private function scenarioSteps(): array
+ {
+ $featureFile = getenv('BDD_FEATURE_FILE') ?: __DIR__ . '/../../scenarios/basic_messaging.feature';
+ assert_true(is_file($featureFile), "feature file not found at {$featureFile}");
+
+ $lines = file($featureFile, FILE_IGNORE_NEW_LINES);
+ assert_true($lines !== false, "failed to read feature file at {$featureFile}");
+
+ $steps = [];
+ $insideTargetScenario = false;
+ foreach ($lines as $line) {
+ $line = trim($line);
+ if ($line === '') {
+ continue;
+ }
+
+ if (str_starts_with($line, 'Scenario:')) {
+ $insideTargetScenario = $line === 'Scenario: Create stream and send messages';
+ continue;
+ }
+
+ if (!$insideTargetScenario) {
+ continue;
+ }
+
+ if (preg_match('/^(Given|When|Then|And) (.+)$/', $line, $matches) === 1) {
+ $steps[] = $matches[2];
+ }
+ }
+
+ assert_true($steps !== [], 'no BDD steps were loaded from the basic messaging scenario');
+
+ return [
+ 'I have a running Iggy server',
+ 'I am authenticated as the root user',
+ ...$steps,
+ ];
+ }
+
+ private function runStep(string $step): void
+ {
+ if ($step === 'I have a running Iggy server') {
+ $this->client = new IggyClient(server_host() . ':' . server_port());
+ $this->client->connect();
+ $this->client->ping();
+
+ return;
+ }
+
+ if ($step === 'I am authenticated as the root user') {
+ $this->requireClient()->loginUser(env_or_default('IGGY_USERNAME', 'iggy'), env_or_default('IGGY_PASSWORD', 'iggy'));
+
+ return;
+ }
+
+ if ($step === 'I have no streams in the system') {
+ return;
+ }
+
+ if (preg_match('/^I create a stream with name "([^"]+)"$/', $step, $matches) === 1) {
+ $streamName = $matches[1];
+ $this->requireClient()->createStream($streamName);
+ $stream = $this->requireClient()->getStream($streamName);
+ assert_not_null($stream);
+ $this->lastStreamId = $stream->id;
+ $this->lastStreamName = $streamName;
+
+ return;
+ }
+
+ if ($step === 'the stream should be created successfully') {
+ assert_not_null($this->requireClient()->getStream($this->requireStreamName()));
+
+ return;
+ }
+
+ if (preg_match('/^the stream should have name "([^"]+)"$/', $step, $matches) === 1) {
+ $stream = $this->requireClient()->getStream($matches[1]);
+ assert_not_null($stream);
+ assert_same($matches[1], $stream->name);
+
+ return;
+ }
+
+ if (preg_match('/^I create a topic with name "([^"]+)" in stream (\d+) with (\d+) partitions$/', $step, $matches) === 1) {
+ $topicName = $matches[1];
+ $streamId = (int) $matches[2];
+ $partitions = (int) $matches[3];
+
+ $this->requireClient()->createTopic($streamId, $topicName, $partitions, null, null, null, null);
+ $topic = $this->requireClient()->getTopic($streamId, $topicName);
+ assert_not_null($topic);
+ $this->lastTopicName = $topicName;
+ $this->lastTopicPartitions = $partitions;
+
+ return;
+ }
+
+ if ($step === 'the topic should be created successfully') {
+ assert_not_null($this->requireClient()->getTopic($this->requireStreamId(), $this->requireTopicName()));
+
+ return;
+ }
+
+ if (preg_match('/^the topic should have name "([^"]+)"$/', $step, $matches) === 1) {
+ $topic = $this->requireClient()->getTopic($this->requireStreamId(), $matches[1]);
+ assert_not_null($topic);
+ assert_same($matches[1], $topic->name);
+
+ return;
+ }
+
+ if (preg_match('/^the topic should have (\d+) partitions$/', $step, $matches) === 1) {
+ $topic = $this->requireClient()->getTopic($this->requireStreamId(), $this->requireTopicName());
+ assert_not_null($topic);
+ assert_same((int) $matches[1], $topic->partitions_count);
+ assert_same((int) $matches[1], $this->lastTopicPartitions);
+
+ return;
+ }
+
+ if (preg_match('/^I send (\d+) messages to stream (\d+), topic (\d+), partition (\d+)$/', $step, $matches) === 1) {
+ $messageCount = (int) $matches[1];
+ $this->lastSentMessageCount = $messageCount;
+ $this->sentPayloads = array_map(
+ static fn (int $index): string => "bdd-php-message-{$index}",
+ range(0, $messageCount - 1),
+ );
+
+ $this->requireClient()->sendMessages(
+ (int) $matches[2],
+ (int) $matches[3],
+ (int) $matches[4],
+ array_map(static fn (string $payload): SendMessage => new SendMessage($payload), $this->sentPayloads),
+ );
+
+ return;
+ }
+
+ if ($step === 'all messages should be sent successfully') {
+ assert_not_null($this->lastSentMessageCount, 'sent message count has not been captured');
+ assert_count($this->lastSentMessageCount, $this->sentPayloads);
+
+ return;
+ }
+
+ if (preg_match('/^I poll messages from stream (\d+), topic (\d+), partition (\d+) starting from offset (\d+)$/', $step, $matches) === 1) {
+ $this->lastPolledMessages = $this->requireClient()->pollMessages(
+ (int) $matches[1],
+ (int) $matches[2],
+ (int) $matches[3],
+ PollingStrategy::offset((int) $matches[4]),
+ 10,
Review Comment:
poll count is a hardcoded `10` while `lastSentMessageCount` is captured a few steps earlier and unused. happens to match the scenario so no live bug, just a magic number - derive it from the sent count instead.
##########
scripts/ci/license-headers.sh:
##########
@@ -185,7 +185,10 @@ find_duplicate_license_headers() {
local path
: > "$output_file"
- mapfile -t LICENSE_EXCLUDES < <(load_license_excludes)
+ LICENSE_EXCLUDES=()
Review Comment:
this mapfile->while-read swap is behavior-identical on bash 4 (it only helps bash 3.2 / older macos) and it's unrelated to adding the php bdd suite.
dont do that in this PR.
##########
.github/config/components.yml:
##########
@@ -307,6 +307,18 @@ components:
- "bdd/scenarios/**"
tasks: ["bdd-python"]
+ bdd-php:
+ depends_on:
+ - "rust-server"
+ - "rust-sdk"
+ - "sdk-php"
+ - "bdd-infrastructure"
+ - "ci-infrastructure"
+ paths:
+ - "bdd/php/**"
+ - "bdd/scenarios/**"
Review Comment:
`bdd/scenarios/**` here means a `leader_redirection.feature` edit triggers the php bdd job even though php only runs `basic_messaging`, so it's wasted ci. mirrors the python component so not new, but worth narrowing if you want the trigger to be accurate.
##########
bdd/php/tests/BasicMessagingFeatureTest.php:
##########
@@ -0,0 +1,278 @@
+<?php
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+declare(strict_types=1);
+
+use Iggy\Client as IggyClient;
+use Iggy\PollingStrategy;
+use Iggy\SendMessage;
+use PHPUnit\Framework\Attributes\TestDox;
+use PHPUnit\Framework\TestCase;
+
+final class BasicMessagingFeatureTest extends TestCase
+{
+ private ?IggyClient $client = null;
+ private ?int $lastStreamId = null;
+ private ?string $lastStreamName = null;
+ private ?string $lastTopicName = null;
+ private ?int $lastTopicPartitions = null;
+ private array $lastPolledMessages = [];
+ private array $sentPayloads = [];
+ private ?int $lastSentMessageCount = null;
+
+ #[TestDox('Basic messaging shared BDD scenario passes for the PHP SDK')]
+ public function testBasicMessagingScenario(): void
+ {
+ foreach ($this->scenarioSteps() as $step) {
+ $this->runStep($step);
+ }
+ }
+
+ protected function tearDown(): void
+ {
+ if ($this->client !== null && $this->lastStreamName !== null) {
+ cleanup_stream_with_topics(
+ $this->client,
+ $this->lastStreamName,
+ $this->lastTopicName !== null ? [$this->lastTopicName] : [],
+ );
+ }
+
+ parent::tearDown();
+ }
+
+ private function scenarioSteps(): array
+ {
+ $featureFile = getenv('BDD_FEATURE_FILE') ?: __DIR__ . '/../../scenarios/basic_messaging.feature';
+ assert_true(is_file($featureFile), "feature file not found at {$featureFile}");
+
+ $lines = file($featureFile, FILE_IGNORE_NEW_LINES);
+ assert_true($lines !== false, "failed to read feature file at {$featureFile}");
+
+ $steps = [];
+ $insideTargetScenario = false;
+ foreach ($lines as $line) {
+ $line = trim($line);
+ if ($line === '') {
+ continue;
+ }
+
+ if (str_starts_with($line, 'Scenario:')) {
+ $insideTargetScenario = $line === 'Scenario: Create stream and send messages';
+ continue;
+ }
+
+ if (!$insideTargetScenario) {
+ continue;
+ }
+
+ if (preg_match('/^(Given|When|Then|And) (.+)$/', $line, $matches) === 1) {
+ $steps[] = $matches[2];
+ }
+ }
+
+ assert_true($steps !== [], 'no BDD steps were loaded from the basic messaging scenario');
+
+ return [
Review Comment:
the `Background` steps are hardcoded and prepended manually here. the parser never reads the `Background` block (it skips everything before the `Scenario:` line), so if a background step is added or changed in the shared feature, php won't pick it up and the test stays green. drive the background off the parsed feature instead of hardcoding it.
##########
bdd/php/Dockerfile:
##########
@@ -0,0 +1,66 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+FROM rust:1.95-slim-trixie
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ ca-certificates \
+ clang \
+ composer \
+ curl \
+ git \
+ libclang-dev \
+ libssl-dev \
+ php-cli \
+ php-dev \
+ php-mbstring \
+ php-xml \
+ pkg-config \
+ unzip \
+ && rm -rf /var/lib/apt/lists/*
+
+ENV PHP=/usr/bin/php
+ENV PHP_CONFIG=/usr/bin/php-config
+ENV IGGY_HOST=iggy-server
+ENV IGGY_PORT=8090
+ENV BDD_FEATURE_FILE=/app/features/basic_messaging.feature
+
+WORKDIR /workspace
+
+COPY Cargo.toml Cargo.lock ./
+COPY core/ ./core/
Review Comment:
`COPY core/` pulls the whole workspace in to build a cdylib that only needs the `core/sdk` closure (`sdk` + `common` + `binary_protocol`). a change to any unrelated core crate busts this layer. you can narrow it to those three crates plus the root `Cargo.toml`/`Cargo.lock` (the root manifest has to stay for workspace resolution) - build-time only, no correctness impact.
##########
bdd/php/Dockerfile:
##########
@@ -0,0 +1,66 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+FROM rust:1.95-slim-trixie
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ ca-certificates \
+ clang \
+ composer \
+ curl \
+ git \
+ libclang-dev \
+ libssl-dev \
+ php-cli \
+ php-dev \
+ php-mbstring \
+ php-xml \
+ pkg-config \
+ unzip \
+ && rm -rf /var/lib/apt/lists/*
+
+ENV PHP=/usr/bin/php
+ENV PHP_CONFIG=/usr/bin/php-config
+ENV IGGY_HOST=iggy-server
+ENV IGGY_PORT=8090
+ENV BDD_FEATURE_FILE=/app/features/basic_messaging.feature
+
+WORKDIR /workspace
+
+COPY Cargo.toml Cargo.lock ./
+COPY core/ ./core/
+
+COPY foreign/php/Cargo.toml ./foreign/php/
+COPY foreign/php/composer.json ./foreign/php/
+COPY foreign/php/phpunit.xml.dist ./foreign/php/
+COPY foreign/php/README.md foreign/php/LICENSE foreign/php/NOTICE ./foreign/php/
+COPY foreign/php/.cargo/ ./foreign/php/.cargo/
+COPY foreign/php/src/ ./foreign/php/src/
+COPY foreign/php/tests/ ./foreign/php/tests/
+COPY foreign/php/scripts/ ./foreign/php/scripts/
+
+COPY bdd/php/ ./bdd/php/
+
+WORKDIR /workspace/foreign/php
+
+RUN cargo install cargo-php --locked --version 0.1.21 \
+ && rm -rf /usr/local/cargo/registry /usr/local/cargo/git
+RUN composer install --no-interaction --prefer-dist
+RUN cargo php install --yes
+RUN chmod +x ./scripts/test.sh
+
+CMD ["./scripts/test.sh", "--configuration", "/workspace/bdd/php/phpunit.xml.dist", "/workspace/bdd/php/tests"]
Review Comment:
the command passes both `--configuration .../phpunit.xml.dist` and a positional `.../bdd/php/tests`. phpunit uses the positional path and ignores the `<testsuites>` block in the config, so that block is dead (harmless - both resolve to the same dir). drop the positional arg (here and in the compose `command`) to keep one source of truth.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(php): add BDD suite wiring [iggy]
Posted by "countradooku (via GitHub)" <gi...@apache.org>.
countradooku commented on PR #3410:
URL: https://github.com/apache/iggy/pull/3410#issuecomment-4621921746
/ready
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org