Convert Behat annotations to PHP attributes in UI Shop contexts#18829
Convert Behat annotations to PHP attributes in UI Shop contexts#18829TheMilek merged 2 commits intoSylius:2.3from
Conversation
📝 WalkthroughWalkthroughMultiple Behat Shop UI context files are migrated from PHPDoc step annotations ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
❗ Preview Environment delete from Bunnyshell failedCheck https://github.com/Sylius/Sylius/actions/runs/22107436203 for details. See: Environment Details Available commands:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Sylius/Behat/Context/Ui/Shop/CurrencyContext.php (1)
73-85:⚠️ Potential issue | 🔴 CriticalBug:
iShouldSeeCurrenciesInTheListhas inverted logic and a brokenin_arraycheck.Three issues:
$currenciesCodesis a variadicstring ...(i.e., an array), but it's passed directly toin_array()— this checks if the array itself is an element of the available currencies list, which will always befalse.- The logic is inverted: the method name and step pattern say "I should see ... in the list," but the code throws when the currencies are found.
- Line 81 passes an array to
sprintf('%s', ...), which will produce a PHP warning/error.This method should assert that each currency is present, not that it isn't.
🐛 Proposed fix
#[Then('I should see :firstCurrency and :secondCurrency in the list')] public function iShouldSeeCurrenciesInTheList(string ...$currenciesCodes): void { $this->homePage->open(); - if (in_array($currenciesCodes, $this->homePage->getAvailableCurrencies(), true)) { - throw new \InvalidArgumentException(sprintf( - 'Expected "%s" not to be in "%s"', - $currenciesCodes, - implode('", "', $this->homePage->getAvailableCurrencies()), - )); - } + $availableCurrencies = $this->homePage->getAvailableCurrencies(); + foreach ($currenciesCodes as $currencyCode) { + if (!in_array($currencyCode, $availableCurrencies, true)) { + throw new \InvalidArgumentException(sprintf( + 'Expected "%s" to be in "%s"', + $currencyCode, + implode('", "', $availableCurrencies), + )); + } + } }src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutThankYouContext.php (1)
67-71:⚠️ Potential issue | 🟡 MinorStep text contradicts assertion logic.
The step pattern says
I should see the thank you page in :localeCode, but the method assertsAssert::false(...)— meaning it expects the page to not be open in that locale. If this is intentional (e.g., verifying locale restriction), the step text should say "I should not see the thank you page in :localeCode" to match.src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php (1)
355-359:⚠️ Potential issue | 🟡 Minor
#[When]attribute used for an assertion method (iShouldSeeWithUnitPrice).Line 355 declares
#[When(...)]but the method nameiShouldSeeWithUnitPriceand its body (Assert::same(...)) indicate it's an assertion — this should likely be#[Then].Proposed fix
- #[When('/^I should see (product "[^"]+") with unit price ("[^"]+")$/')] + #[Then('/^I should see (product "[^"]+") with unit price ("[^"]+")$/')]
🤖 Fix all issues with AI agents
In `@src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php`:
- Around line 195-199: The step definition attribute on the method
myTaxTotalShouldBe in class CheckoutCompleteContext uses #[Given('my tax total
should be :taxTotal')] but this is an assertion step and should be annotated
with #[Then(...)]; update the attribute to #[Then('my tax total should be
:taxTotal')] (keeping the method name myTaxTotalShouldBe and the Assert::same
call intact) so the step follows the Given-When-Then semantics and Behat will
recognize it as a Then assertion.
🧹 Nitpick comments (11)
rector.php (1)
12-15: Commented-out Rector sets should be tracked for re-enablement.Disabling
UP_TO_PHP_82andPHPUNIT_110sets removes automated upgrade rules from the pipeline. If this is temporary (e.g., to avoid conflicts during the attribute migration), consider adding a TODO comment so the sets are re-enabled once the migration is complete.src/Sylius/Behat/Context/Ui/Shop/AddressBookContext.php (1)
16-18: Import order nit:Behat\Step\Thenshould come beforeBehat\Step\Whenalphabetically.Current order is
Given,When,Then; alphabetical would beGiven,Then,When. This is consistent with other files in this PR (e.g.,CheckoutCompleteContext.phplines 16-20 has them sorted), but inconsistent here.Proposed fix
use Behat\Step\Given; -use Behat\Step\When; use Behat\Step\Then; +use Behat\Step\When;src/Sylius/Behat/Context/Ui/Shop/BrowsingProductContext.php (1)
16-17: Imports are not sorted alphabetically.
Behat\Behat\Context\Contextshould precedeBehat\Step\Thenalphabetically.Proposed fix
-use Behat\Step\Then; use Behat\Behat\Context\Context; +use Behat\Step\Then;As per coding guidelines: "Sort PHP
useimports alphabetically and group by type (classes, functions, constants)".src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutPaymentContext.php (1)
16-20: Imports are not sorted alphabetically.The new
Behat\Step\GivenandBehat\Step\Thenimports were added aboveBehat\Behat\Context\Contextand are not contiguous with the existingBehat\Step\Whenon line 20.Proposed fix
-use Behat\Step\Given; -use Behat\Step\Then; use Behat\Behat\Context\Context; use Behat\Mink\Exception\ElementNotFoundException; +use Behat\Step\Given; +use Behat\Step\Then; use Behat\Step\When;As per coding guidelines: "Sort PHP
useimports alphabetically and group by type (classes, functions, constants)".src/Sylius/Behat/Context/Ui/Shop/HomepageContext.php (1)
16-18: Imports are not sorted alphabetically.Same pattern as other files —
Behat\Step\*imports should come afterBehat\Behat\Context\Context.Proposed fix
-use Behat\Step\When; -use Behat\Step\Then; use Behat\Behat\Context\Context; +use Behat\Step\Then; +use Behat\Step\When;As per coding guidelines: "Sort PHP
useimports alphabetically and group by type (classes, functions, constants)".src/Sylius/Behat/Context/Ui/Shop/LocaleContext.php (1)
16-19: Imports are not sorted alphabetically.Proposed fix
-use Behat\Step\Given; -use Behat\Step\When; -use Behat\Step\Then; use Behat\Behat\Context\Context; +use Behat\Step\Given; +use Behat\Step\Then; +use Behat\Step\When;As per coding guidelines: "Sort PHP
useimports alphabetically and group by type (classes, functions, constants)".src/Sylius/Behat/Context/Ui/Shop/ProductAttributeContext.php (1)
16-17: Imports are not sorted alphabetically.Proposed fix
-use Behat\Step\Then; use Behat\Behat\Context\Context; +use Behat\Step\Then;As per coding guidelines: "Sort PHP
useimports alphabetically and group by type (classes, functions, constants)".src/Sylius/Behat/Context/Ui/Shop/RegistrationContext.php (1)
16-18: Imports are not sorted alphabetically.Proposed fix
-use Behat\Step\Then; use Behat\Behat\Context\Context; +use Behat\Step\Then; use Behat\Step\When;As per coding guidelines: "Sort PHP
useimports alphabetically and group by type (classes, functions, constants)".src/Sylius/Behat/Context/Ui/Shop/Checkout/RegistrationAfterCheckoutContext.php (1)
16-18: Imports are not sorted alphabetically.Proposed fix
-use Behat\Step\Then; use Behat\Behat\Context\Context; +use Behat\Step\Then; use Behat\Step\When;As per coding guidelines: "Sort PHP
useimports alphabetically and group by type (classes, functions, constants)".src/Sylius/Behat/Context/Ui/Shop/AccountContext.php (1)
16-19: Imports are not sorted alphabetically.Proposed fix
-use Behat\Step\When; -use Behat\Step\Then; -use Behat\Step\Given; use Behat\Behat\Context\Context; +use Behat\Step\Given; +use Behat\Step\Then; +use Behat\Step\When;As per coding guidelines: "Sort PHP
useimports alphabetically and group by type (classes, functions, constants)".src/Sylius/Behat/Context/Ui/Shop/CheckoutContext.php (1)
16-19: Import statements are not sorted alphabetically.The newly added
use Behat\Step\Thenwas inserted beforeuse Behat\Behat\Context\Context, breaking alphabetical order. As per coding guidelines,useimports should be sorted alphabetically.Suggested fix
-use Behat\Step\Then; use Behat\Behat\Context\Context; use Behat\Step\Given; +use Behat\Step\Then; use Behat\Step\When;
| #[Given('my tax total should be :taxTotal')] | ||
| public function myTaxTotalShouldBe(string $taxTotal): void | ||
| { | ||
| Assert::same($this->completePage->getTaxTotal(), $taxTotal); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "my tax total should be" --glob '*.feature' -C2Repository: Sylius/Sylius
Length of output: 2337
Change #[Given] to #[Then] for assertion step.
This step is used exclusively as an assertion after Then steps in feature files (e.g., "And my tax total should be..."). It should be annotated with #[Then], not #[Given], to comply with the Given-When-Then pattern.
🤖 Prompt for AI Agents
In `@src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php` around
lines 195 - 199, The step definition attribute on the method myTaxTotalShouldBe
in class CheckoutCompleteContext uses #[Given('my tax total should be
:taxTotal')] but this is an assertion step and should be annotated with
#[Then(...)]; update the attribute to #[Then('my tax total should be
:taxTotal')] (keeping the method name myTaxTotalShouldBe and the Assert::same
call intact) so the step follows the Given-When-Then semantics and Behat will
recognize it as a Then assertion.
src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php
Outdated
Show resolved
Hide resolved
src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutPaymentContext.php
Outdated
Show resolved
Hide resolved
src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutShippingContext.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Kamil Grygierzec <53942444+TheMilek@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php (1)
355-359:⚠️ Potential issue | 🟡 MinorChange
#[When]to#[Then]— this is an assertion step, not an action.The pattern
I should see ... with unit price ...is assertion language and performsAssert::same. Feature files use this step asAnd(an assertion continuation), not as aWhenaction. It should be annotated with#[Then].Proposed fix
- #[When('/^I should see (product "[^"]+") with unit price ("[^"]+")$/')] + #[Then('/^I should see (product "[^"]+") with unit price ("[^"]+")$/')]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php` around lines 355 - 359, The step definition iShouldSeeWithUnitPrice currently annotated with #[When] is an assertion and should be #[Then]; update the attribute on the iShouldSeeWithUnitPrice method in CheckoutCompleteContext.php (method name: iShouldSeeWithUnitPrice, uses Assert::same and calls $this->completePage->getProductUnitPrice) to #[Then('/^I should see (product "[^"]+") with unit price ("[^"]+")$/')] so it reflects assertion semantics in feature files.src/Sylius/Behat/Context/Ui/Shop/CurrencyContext.php (1)
73-85:⚠️ Potential issue | 🔴 CriticalBug:
iShouldSeeCurrenciesInTheListhas inverted logic and a type error.Three problems:
- Inverted assertion: The step text
I should see :firstCurrency and :secondCurrency in the listexpects the currencies to be present, but the code throws when they are found — this is backwards.- Array passed to
in_arrayas needle:$currenciesCodesis a variadicarray, not a string.in_array($currenciesCodes, $availableCurrencies, true)will almost never match because it checks whether the array itself is an element. Each currency code should be checked individually.sprintf('%s', $currenciesCodes)on Line 81 passes an array to%s, producing"Array"and a PHP warning.Proposed fix
#[Then('I should see :firstCurrency and :secondCurrency in the list')] public function iShouldSeeCurrenciesInTheList(string ...$currenciesCodes): void { $this->homePage->open(); - if (in_array($currenciesCodes, $this->homePage->getAvailableCurrencies(), true)) { - throw new \InvalidArgumentException(sprintf( - 'Expected "%s" not to be in "%s"', - $currenciesCodes, - implode('", "', $this->homePage->getAvailableCurrencies()), - )); - } + $availableCurrencies = $this->homePage->getAvailableCurrencies(); + + foreach ($currenciesCodes as $currencyCode) { + Assert::oneOf($currencyCode, $availableCurrencies); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Sylius/Behat/Context/Ui/Shop/CurrencyContext.php` around lines 73 - 85, The step iShouldSeeCurrenciesInTheList currently uses inverted logic and treats the variadic $currenciesCodes as a single needle; fix it by fetching $available = $this->homePage->getAvailableCurrencies() after opening the page, then loop over each currency code in the variadic $currenciesCodes and assert in_array($code, $available, true); if any are missing throw an \InvalidArgumentException listing the missing codes (use implode to format the message) so you check presence (not absence) and avoid passing an array to sprintf/%s.
🧹 Nitpick comments (2)
src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php (1)
16-20: Imports are not sorted alphabetically.
Behat\Step\Given(line 16) should come afterBehat\Mink\Exception\ElementNotFoundException(line 18) per alphabetical ordering.Proposed fix
-use Behat\Step\Given; use Behat\Behat\Context\Context; use Behat\Mink\Exception\ElementNotFoundException; +use Behat\Step\Given; use Behat\Step\Then; use Behat\Step\When;As per coding guidelines: "Sort PHP
useimports alphabetically and group by type (classes, functions, constants)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php` around lines 16 - 20, The import statements in CheckoutCompleteContext are not alphabetically ordered; reorder the use imports so they are sorted alphabetically (grouping classes together) — specifically ensure Behat\Mink\Exception\ElementNotFoundException comes before Behat\Step\Given and that Behat\Step\Then and Behat\Step\When are placed in proper alphabetical order relative to the others; update the use block containing Behat\Step\Given, Behat\Behat\Context\Context, Behat\Mink\Exception\ElementNotFoundException, Behat\Step\Then, and Behat\Step\When to the correct alphabetical sequence.src/Sylius/Behat/Context/Ui/Shop/CurrencyContext.php (1)
59-71: Minor: hand-rolled assertion duplicatesAssert::notOneOfsemantics.This works correctly but could use
Webmozart\Assert\Assertfor consistency with the rest of the file (e.g., Line 56 usesAssert::oneOf). Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Sylius/Behat/Context/Ui/Shop/CurrencyContext.php` around lines 59 - 71, Replace the hand-rolled existence check in iShouldNotBeAbleToShopUsingTheCurrency with Webmozart\Assert::notOneOf to match file convention; call Assert::notOneOf($currencyCode, $this->homePage->getAvailableCurrencies(), 'Expected "%s" not to be available') so the assertion uses the same Assert utility as the one used elsewhere (see Assert::oneOf usage) and remove the manual in_array/throw logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php`:
- Around line 355-359: The step definition iShouldSeeWithUnitPrice currently
annotated with #[When] is an assertion and should be #[Then]; update the
attribute on the iShouldSeeWithUnitPrice method in CheckoutCompleteContext.php
(method name: iShouldSeeWithUnitPrice, uses Assert::same and calls
$this->completePage->getProductUnitPrice) to #[Then('/^I should see (product
"[^"]+") with unit price ("[^"]+")$/')] so it reflects assertion semantics in
feature files.
In `@src/Sylius/Behat/Context/Ui/Shop/CurrencyContext.php`:
- Around line 73-85: The step iShouldSeeCurrenciesInTheList currently uses
inverted logic and treats the variadic $currenciesCodes as a single needle; fix
it by fetching $available = $this->homePage->getAvailableCurrencies() after
opening the page, then loop over each currency code in the variadic
$currenciesCodes and assert in_array($code, $available, true); if any are
missing throw an \InvalidArgumentException listing the missing codes (use
implode to format the message) so you check presence (not absence) and avoid
passing an array to sprintf/%s.
---
Duplicate comments:
In `@src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php`:
- Around line 195-199: Change the Behat step attribute on the myTaxTotalShouldBe
method in CheckoutCompleteContext from #[Given('my tax total should be
:taxTotal')] to #[Then('my tax total should be :taxTotal')]; the method
(myTaxTotalShouldBe) performs an assertion using Assert::same so it should be
annotated with Then instead of Given to follow Given-When-Then semantics,
leaving the method body and parameter unchanged.
---
Nitpick comments:
In `@src/Sylius/Behat/Context/Ui/Shop/Checkout/CheckoutCompleteContext.php`:
- Around line 16-20: The import statements in CheckoutCompleteContext are not
alphabetically ordered; reorder the use imports so they are sorted
alphabetically (grouping classes together) — specifically ensure
Behat\Mink\Exception\ElementNotFoundException comes before Behat\Step\Given and
that Behat\Step\Then and Behat\Step\When are placed in proper alphabetical order
relative to the others; update the use block containing Behat\Step\Given,
Behat\Behat\Context\Context, Behat\Mink\Exception\ElementNotFoundException,
Behat\Step\Then, and Behat\Step\When to the correct alphabetical sequence.
In `@src/Sylius/Behat/Context/Ui/Shop/CurrencyContext.php`:
- Around line 59-71: Replace the hand-rolled existence check in
iShouldNotBeAbleToShopUsingTheCurrency with Webmozart\Assert::notOneOf to match
file convention; call Assert::notOneOf($currencyCode,
$this->homePage->getAvailableCurrencies(), 'Expected "%s" not to be available')
so the assertion uses the same Assert utility as the one used elsewhere (see
Assert::oneOf usage) and remove the manual in_array/throw logic.
Summary by CodeRabbit
Release Notes