[#285] Updated FieldTrait. #351
Conversation
WalkthroughThe changes standardize and update step definitions from version 2 to version 3. In the migration map, revised phrasing for field and keyboard assertions replaces less clear language and removes redundant steps. The trait in the source code now uses updated documentation and method signatures (e.g., renaming a parameter from Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (3)
🧰 Additional context used🪛 LanguageToolMIGRATION.md[grammar] ~31-~31: There is an agreement error between ‘see’ and ‘field’. Insert ‘a(n)’ or change the noun to plural. (PRP_VB_NN) 🪛 markdownlint-cli2 (0.17.2)MIGRATION.md39-39: Table pipe style (MD055, table-pipe-style) 39-39: Table column count (MD056, table-column-count) 🪛 GitHub Check: codecov/patchsrc/FieldTrait.php[warning] 41-41: src/FieldTrait.php#L41 [warning] 66-66: src/FieldTrait.php#L66 [warning] 86-86: src/FieldTrait.php#L86 [warning] 89-89: src/FieldTrait.php#L89 [warning] 151-151: src/FieldTrait.php#L151 [warning] 166-166: src/FieldTrait.php#L166 [warning] 174-175: src/FieldTrait.php#L174-L175 [warning] 177-177: src/FieldTrait.php#L177 🔇 Additional comments (17)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #351 +/- ##
==========================================
- Coverage 79.64% 79.55% -0.10%
==========================================
Files 31 30 -1
Lines 1474 1467 -7
==========================================
- Hits 1174 1167 -7
Misses 300 300 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
There was a problem hiding this comment.
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
Files not reviewed (2)
- src/FieldTrait.php: Language not supported
- tests/behat/features/field.feature: Language not supported
Comments suppressed due to low confidence (2)
steps.md:400
- [nitpick] Consider using consistent step definition phrasing across existence checks; verify that the term 'the field' aligns with the naming conventions used in other step definitions.
@Then the field :name should exist
MIGRATION.md:35
- [nitpick] The updated field state mapping is now generic; consider clarifying in the documentation how specific states (e.g., 'disabled', 'enabled') are represented to avoid ambiguity.
Then field :name is :disabled on the page
→
Then the field :name should be :state
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MIGRATION.md (1)
38-38: Fix missing table formattingThe table row is missing the trailing pipe character, which is causing markdown formatting issues according to static analysis.
-| +| |🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
38-38: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe(MD055, table-pipe-style)
38-38: Table column count
Expected: 2; Actual: 1; Too few cells, row will be missing data(MD056, table-column-count)
tests/behat/features/field.feature (1)
82-93: Duplicate error test scenarioThis scenario appears to be testing the same condition as the previous one, just using the field ID instead of the field label. Consider adding a comment to clarify the difference between these two scenarios or consolidating them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MIGRATION.md(2 hunks)src/FieldTrait.php(4 hunks)steps.md(1 hunks)tests/behat/features/field.feature(4 hunks)
🧰 Additional context used
🪛 LanguageTool
MIGRATION.md
[grammar] ~31-~31: There is an agreement error between ‘see’ and ‘field’. Insert ‘a(n)’ or change the noun to plural.
Context: ... | | Then I see field :name ...
(PRP_VB_NN)
🪛 markdownlint-cli2 (0.17.2)
MIGRATION.md
38-38: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
38-38: Table column count
Expected: 2; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
🔇 Additional comments (24)
steps.md (6)
400-401: Well-structured change to field existence assertion phraseThe change from "I see field" to "the field should exist" is a good improvement that aligns with BDD best practices by using more declarative phrasing rather than imperative.
404-406: Consistently updated examples to match new assertion styleThe examples properly reflect the updated step definition syntax, maintaining consistency throughout the documentation.
411-412: Improvement to field non-existence assertion phraseSimilar to the existence assertion, this change to "should not exist" follows the same pattern of making the step definition more declarative and consistent.
415-417: Examples correctly updated for non-existence assertionThe examples properly mirror the updated step definition, maintaining documentation consistency.
422-423: Better state assertion with improved clarityThe change from "field is disabled" to "field should be state" makes the assertion more versatile and descriptive, allowing for various states beyond just "disabled".
426-430: Comprehensive examples for different state assertionsThe examples now clearly demonstrate multiple state conditions ("disabled", "enabled", "not enabled"), which helps users understand how to use this assertion in different scenarios.
MIGRATION.md (2)
31-37: Comprehensive migration map for field-related stepsThe migration map correctly documents all the changes from v2 to v3 for the FieldTrait, including the removed steps and the renamed states. This will be very helpful for users upgrading from v2 to v3 of the library.
🧰 Tools
🪛 LanguageTool
[grammar] ~31-~31: There is an agreement error between ‘see’ and ‘field’. Insert ‘a(n)’ or change the noun to plural.
Context: ... | |Then I see field :name...(PRP_VB_NN)
52-54: Consistent terminology in link trait migrationThe LinkTrait migration entries now use consistent terminology with the rest of the migration map, focusing on "should be" rather than "is" phrases.
src/FieldTrait.php (6)
23-25: Updated documentation comments to reflect new assertion styleDocumentation now matches the step definition changes, improving consistency between code and documentation.
Also applies to: 27-28
49-51: Consistent documentation update for not-exists assertionsThe documentation for the non-existence assertion method follows the same pattern as the existence assertion, maintaining a consistent style throughout the trait.
Also applies to: 53-54
70-74: Improved documentation for state assertionsThe documentation now clearly shows different state examples, making it more obvious how to use this method for both positive and negative assertions about field states.
Also applies to: 76-77
78-87: Method parameter name change from $disabled to $stateThe parameter renaming from
$disabledto$statemakes the method more flexible and better reflects its purpose of handling various states. The implementation correctly uses the new parameter name in its conditions.
93-94: Added alias annotation for color field methodAdding the alternative annotation pattern maintains backward compatibility while introducing the new, more descriptive step definition.
113-114: Updated annotation for color field assertionThe color field assertion annotation now follows the same pattern as other assertions in the trait, improving consistency.
tests/behat/features/field.feature (10)
6-8: Updated test scenario to use new step definition syntaxThe test scenario now correctly uses the new "should exist" syntax, ensuring that the tests validate the updated implementation.
12-13: Updated non-existence assertion in testThe test for field non-existence now uses the updated step definition syntax.
17-25: Updated scenario outline for field existenceThe scenario outline now uses the new syntax with examples that properly test both existence and non-existence conditions.
29-38: Updated scenario outline for field stateThe scenario outline for testing field states now uses the more flexible "should be" syntax with examples for both enabled and disabled states.
42-45: Updated assertions for color fieldsThe color field assertions now use the new syntax, maintaining consistency with other field-related assertions.
49-52: Added test for alternative color field step definitionA new test scenario has been added to test the alternative step definition for filling in color fields, ensuring both annotation patterns work correctly.
54-65: Updated error test scenario for field existenceThe test scenario for verifying error messages when field existence fails has been updated to use the new syntax, ensuring that error handling still works correctly.
68-79: Updated error test scenario for field non-existenceSimilar to the existence error test, this scenario has been updated to use the new syntax while verifying the same error message is produced.
96-108: Updated state error test scenarioThe test for verifying error messages when field state assertions fail has been updated to use the new syntax, ensuring error handling works correctly with the renamed parameter.
110-121: Updated negative assertion test for field stateThe final test scenario for field state negative assertion has been updated to use the new syntax while ensuring the error message is still correctly produced.
There was a problem hiding this comment.
@skipper-vp
Thank you for working on this.
Please my comments and update.
Also, please bring here the WYSIWYG steps.
Thank you
src/FieldTrait.php
Outdated
| @@ -151,7 +110,7 @@ public function fillColorField(string $field, ?string $value = NULL): mixed { | |||
| /** | |||
| * Asserts that a color field has a value. | |||
| * | |||
| * @Then /^color field "(?P<field>(?:[^"]|\\")*)" value is "(?P<value>(?:[^"]|\\")*)"$/ | |||
| * @Then /^the color field "(?P<field>(?:[^"]|\\")*)" should have the value "(?P<value>(?:[^"]|\\")*)"$/ | |||
There was a problem hiding this comment.
please replace the regexp according to #134 (comment) 1.a
There was a problem hiding this comment.
Replaced with @Then the color field :field should have the value :value
src/FieldTrait.php
Outdated
| /** | ||
| * Fills value for color field. | ||
| * | ||
| * @When /^(?:|I )fill color in "(?P<field>(?:[^"]|\\")*)" with "(?P<value>(?:[^"]|\\")*)"$/ | ||
| * @When /^(?:|I )fill color in "(?P<value>(?:[^"]|\\")*)" for "(?P<field>(?:[^"]|\\")*)"$/ | ||
| * @When /^(?:|I )fill in the color field "(?P<field>(?:[^"]|\\")*)" with the value "(?P<value>(?:[^"]|\\")*)"$/ |
There was a problem hiding this comment.
please replace the regexp according to #134 (comment) 1.a
There was a problem hiding this comment.
Replaced with
* @When I fill color in :field with :value
* @When I fill in the color field :field with the value :value
src/FieldTrait.php
Outdated
| * @endcode | ||
| * | ||
| * @Then I don't see field :name | ||
| * @Then the field :name should not exist | ||
| */ | ||
| public function fieldAssertNotExists(string $field_name): void { |
There was a problem hiding this comment.
please update the arg name as per please replace the regexp according to #134 (comment) 1.d
There was a problem hiding this comment.
Updated with public function fieldAssertNotExists(string $name): void {
src/FieldTrait.php
Outdated
| */ | ||
| public function fieldAssertState(string $field_name, string $disabled): void { | ||
| public function fieldAssertState(string $field_name, string $state): void { |
There was a problem hiding this comment.
please update the arg name according to #134 (comment) 1.d
There was a problem hiding this comment.
Updated with public function fieldAssertState(string $name, string $enabled_or_disabled): void {
src/FieldTrait.php
Outdated
| * @endcode | ||
| * | ||
| * @Then field :name is :disabled on the page | ||
| * @Then the field :name should be :state |
There was a problem hiding this comment.
please update this to
@Then the field :name should be :enabled_or_disabled
and then also please update it everywhere
There was a problem hiding this comment.
This has been updated
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Closes #285
Updated FieldTrait.
Summary by CodeRabbit
Documentation
Refactor
Tests