Add extended failure message for visual testing#6445
Conversation
📝 WalkthroughWalkthroughIntroduces a new configurable property Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@vividus-extension-visual-testing/src/main/java/org/vividus/visual/steps/AbstractVisualSteps.java`:
- Line 40: The field and accessor are misspelled using "Assertation" instead of
"Assertion": rename the boolean field enableExtendedAssertationFormat to
enableExtendedAssertionFormat and rename the setter
setEnableExtendedAssertationFormat(...) to
setEnableExtendedAssertionFormat(...); update all usages including the Spring
XML property name (enable-extended-assertion-format /
enableExtendedAssertionFormat) and the test method
shouldUseExtendedAssertationFormat → shouldUseExtendedAssertionFormat to keep
the bean property API consistent.
In
`@vividus-extension-visual-testing/src/test/java/org/vividus/visual/steps/VisualStepsTests.java`:
- Around line 153-164: Convert this single-case test into a `@ParameterizedTest`
using `@CsvSource` to iterate over VisualActionType values (COMPARE_AGAINST,
CHECK_INEQUALITY_AGAINST) and passed flag (true/false); for each combination
create the AbstractVisualCheck with BASELINE_NAME, set
visualCheckResult.setPassed(passed), call
visualSteps.setEnableExtendedAssertionFormat(true) and
visualSteps.verifyResult(visualCheckResult), then compute the expected assertion
outcome using the XOR rule from AbstractVisualSteps (i.e., expected = passed ^
(action == CHECK_INEQUALITY_AGAINST)) and verify the softAssert with
softAssert.assertTrue/assertFalse("Visual check for '%s'
baseline".formatted(BASELINE_NAME), expected) so the test covers both action
types and pass/fail states.
---
Duplicate comments:
In
`@vividus-extension-visual-testing/src/main/resources/vividus-extension/spring.xml`:
- Around line 9-11: The XML bean property name enableExtendedAssertionFormat in
the abstractVisualSteps bean does not match the corrected Java field/setter name
in org.vividus.visual.steps.AbstractVisualSteps (you indicated a typo in
AbstractVisualSteps.java); update this property name to the exact corrected Java
property/setter identifier (after you fix the field/setter name in
AbstractVisualSteps) so Spring property wiring uses the new name and remove the
mismatched "Assertion" variant; reference bean name "abstractVisualSteps" and
class "org.vividus.visual.steps.AbstractVisualSteps" when making the change.
| private final IAttachmentPublisher attachmentPublisher; | ||
| private final ISoftAssert softAssert; | ||
|
|
||
| private boolean enableExtendedAssertionFormat; |
There was a problem hiding this comment.
Typo: "Assertion" → "Assertion" in field and setter names.
enableExtendedAssertionFormat / setEnableExtendedAssertionFormat — "Assertion" is not a valid English word. This typo will be baked into the bean property API surface. Since the external property name (enable-extended-assertion-format) is already spelled correctly, fixing the Java identifiers now avoids a breaking rename later.
Proposed fix
- private boolean enableExtendedAssertionFormat;
+ private boolean enableExtendedAssertionFormat;Rename throughout:
- Field:
enableExtendedAssertionFormat→enableExtendedAssertionFormat… wait, let me be precise: enableExtendedAssertionFormat→enableExtendedAssertionFormat
- private boolean enableExtendedAssertionFormat;
+ private boolean enableExtendedAssertionFormat;Apologies — corrected diff:
- private boolean enableExtendedAssertionFormat;
+ private boolean enableExtendedAssertionFormat;Rename Assertion → Assertion in:
- Field
enableExtendedAssertionFormat(line 40) - Setter
setEnableExtendedAssertionFormat(line 112) - Spring XML property name
enableExtendedAssertionFormatinspring.xml - Test method
shouldUseExtendedAssertionFormatinVisualStepsTests.java
Also applies to: 112-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@vividus-extension-visual-testing/src/main/java/org/vividus/visual/steps/AbstractVisualSteps.java`
at line 40, The field and accessor are misspelled using "Assertation" instead of
"Assertion": rename the boolean field enableExtendedAssertationFormat to
enableExtendedAssertionFormat and rename the setter
setEnableExtendedAssertationFormat(...) to
setEnableExtendedAssertionFormat(...); update all usages including the Spring
XML property name (enable-extended-assertion-format /
enableExtendedAssertionFormat) and the test method
shouldUseExtendedAssertationFormat → shouldUseExtendedAssertionFormat to keep
the bean property API consistent.
| @Test | ||
| void shouldUseExtendedAssertionFormat() | ||
| { | ||
| var visualCheck = new AbstractVisualCheck(BASELINE_NAME, VisualActionType.COMPARE_AGAINST) { }; | ||
| var visualCheckResult = new VisualCheckResult(visualCheck); | ||
| visualCheckResult.setPassed(false); | ||
|
|
||
| visualSteps.setEnableExtendedAssertionFormat(true); | ||
| visualSteps.verifyResult(visualCheckResult); | ||
|
|
||
| verify(softAssert).assertTrue("Visual check for '%s' baseline".formatted(BASELINE_NAME), false); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider parameterizing this test to cover both action types and pass/fail states.
Currently only tests COMPARE_AGAINST with passed=false. The extended format interacts with the XOR logic on line 94 of AbstractVisualSteps, so covering CHECK_INEQUALITY_AGAINST and passed=true would strengthen confidence.
Suggested approach
Convert to a @ParameterizedTest with @CsvSource similar to the existing shouldPublishAttachment test:
- `@Test`
- void shouldUseExtendedAssertionFormat()
- {
- var visualCheck = new AbstractVisualCheck(BASELINE_NAME, VisualActionType.COMPARE_AGAINST) { };
- var visualCheckResult = new VisualCheckResult(visualCheck);
- visualCheckResult.setPassed(false);
-
- visualSteps.setEnableExtendedAssertionFormat(true);
- visualSteps.verifyResult(visualCheckResult);
-
- verify(softAssert).assertTrue("Visual check for '%s' baseline".formatted(BASELINE_NAME), false);
- }
+ `@ParameterizedTest`
+ `@CsvSource`({ COMPARE_AGAINST, CHECK_INEQUALITY_AGAINST })
+ void shouldUseExtendedAssertionFormat(boolean passed, VisualActionType action)
+ {
+ var visualCheck = new AbstractVisualCheck(BASELINE_NAME, action) { };
+ var visualCheckResult = new VisualCheckResult(visualCheck);
+ visualCheckResult.setPassed(passed);
+
+ visualSteps.setEnableExtendedAssertionFormat(true);
+ visualSteps.verifyResult(visualCheckResult);
+
+ verify(softAssert).assertTrue("Visual check for '%s' baseline".formatted(BASELINE_NAME), true);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@vividus-extension-visual-testing/src/test/java/org/vividus/visual/steps/VisualStepsTests.java`
around lines 153 - 164, Convert this single-case test into a `@ParameterizedTest`
using `@CsvSource` to iterate over VisualActionType values (COMPARE_AGAINST,
CHECK_INEQUALITY_AGAINST) and passed flag (true/false); for each combination
create the AbstractVisualCheck with BASELINE_NAME, set
visualCheckResult.setPassed(passed), call
visualSteps.setEnableExtendedAssertionFormat(true) and
visualSteps.verifyResult(visualCheckResult), then compute the expected assertion
outcome using the XOR rule from AbstractVisualSteps (i.e., expected = passed ^
(action == CHECK_INEQUALITY_AGAINST)) and verify the softAssert with
softAssert.assertTrue/assertFalse("Visual check for '%s'
baseline".formatted(BASELINE_NAME), expected) so the test covers both action
types and pass/fail states.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6445 +/- ##
=========================================
Coverage 97.77% 97.77%
- Complexity 7397 7400 +3
=========================================
Files 1014 1014
Lines 21405 21413 +8
Branches 1403 1405 +2
=========================================
+ Hits 20928 20936 +8
Misses 361 361
Partials 116 116 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Qodana for JVM7 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| <bean class="org.vividus.visual.eyes.converter.StringToAccessibilitySettingsConverter" /> | ||
|
|
||
| <bean id="visualTestingSteps" class="org.vividus.visual.eyes.VisualTestingSteps" > | ||
| <bean id="visualTestingSteps" class="org.vividus.visual.eyes.VisualTestingSteps" parent="abstractVisualSteps"> |
Check failure
Code scanning / QDJVM
Incorrect constructor injection in XML Spring bean Error
| <bean class="org.vividus.visual.eyes.converter.StringToAccessibilitySettingsConverter" /> | ||
|
|
||
| <bean id="visualTestingSteps" class="org.vividus.visual.eyes.VisualTestingSteps" > | ||
| <bean id="visualTestingSteps" class="org.vividus.visual.eyes.VisualTestingSteps" parent="abstractVisualSteps"> |
Check failure
Code scanning / QDJVM
Incorrect Spring Core XML-based application context Error
| </bean> | ||
|
|
||
| <bean id="ufgSteps" class="org.vividus.visual.eyes.UfgSteps" /> | ||
| <bean id="ufgSteps" class="org.vividus.visual.eyes.UfgSteps" parent="abstractVisualSteps"/> |
Check failure
Code scanning / QDJVM
Incorrect constructor injection in XML Spring bean Error
| </bean> | ||
|
|
||
| <bean id="ufgSteps" class="org.vividus.visual.eyes.UfgSteps" /> | ||
| <bean id="ufgSteps" class="org.vividus.visual.eyes.UfgSteps" parent="abstractVisualSteps"/> |
Check failure
Code scanning / QDJVM
Incorrect Spring Core XML-based application context Error
| <context:component-scan base-package="org.vividus.visual.storage" /> | ||
|
|
||
| <bean id="visualSteps" class="org.vividus.visual.VisualSteps"> | ||
| <bean id="visualSteps" class="org.vividus.visual.VisualSteps" parent="abstractVisualSteps"> |
Check failure
Code scanning / QDJVM
Incorrect Spring Core XML-based application context Error
vividus-extension-visual-testing/src/main/resources/vividus-extension/spring.xml
Fixed
Show fixed
Hide fixed
| |`<empty>` | ||
| |Used to append scenario based index to a baseline name | ||
|
|
||
| |`ui.visual.enable-extended-assertion-format` |
There was a problem hiding this comment.
| |`ui.visual.enable-extended-assertion-format` | |
| |`ui.visual.extended-assertion-format-enabled` |
| softAssert.assertTrue("Visual check passed", passed); | ||
| if (enableExtendedAssertionFormat) | ||
| { | ||
| softAssert.assertTrue("Visual check for '%s' baseline".formatted(result.getBaselineName()), passed); |
There was a problem hiding this comment.
let's use recordPassedAssertion/recordFailedAssertion
uarlouski
force-pushed
the
visual-message
branch
from
ec1c7ae to
8deb4ad
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@vividus-extension-visual-testing/src/main/java/org/vividus/visual/steps/AbstractVisualSteps.java`:
- Around line 40-41: The field and accessor use a misspelling of "Assertion"
(single-s vs double-s); rename the boolean field extendedAsertionFormatEnabled
to extendedAssertionFormatEnabled and rename the setter
setExtendedAsertionFormatEnabled to setExtendedAssertionFormatEnabled (and any
getter if present), and update all references including Spring XML property
names and test method names to the corrected extendedAssertionFormatEnabled
spelling while leaving the external property key unchanged.
In
`@vividus-extension-visual-testing/src/main/resources/vividus-extension/spring.xml`:
- Around line 9-11: The bean property name extendedAssertionFormatEnabled in the
abstractVisualSteps bean is misspelled and must be corrected to
extendedAssertionFormatEnabled -> extendedAssertionFormatEnabled (fix the Java
identifier to match the correctly spelled "Assertion"); update the property name
in the spring.xml bean declaration to the corrected identifier and then rename
the corresponding field and its setter (and getter if present) in the
AbstractVisualSteps class (e.g., field extendedAssertionFormatEnabled and
setExtendedAssertionFormatEnabled) so they match the corrected property name;
keep the external property key ui.visual.extended-assertion-format-enabled
unchanged and ensure Spring injection still maps to the renamed Java property.
| default-lazy-init="true"> | ||
|
|
||
| <bean name="abstractVisualSteps" class="org.vividus.visual.steps.AbstractVisualSteps" abstract="true"> | ||
| <property name="extendedAssertionFormatEnabled" value="${ui.visual.extended-assertion-format-enabled}" /> |
Check warning
Code scanning / QDJVM
Unresolved placeholders configured in the Spring XML application context Warning
There was a problem hiding this comment.
Pull request overview
This pull request adds an optional extended assertion message format for visual testing that includes the baseline name in assertion messages. The feature is disabled by default to maintain backward compatibility.
Changes:
- Added a new configuration property
ui.visual.extended-assertion-format-enabled(default: false) to enable extended assertion messages - Implemented extended assertion format in
AbstractVisualStepsthat displays "Visual check for '' baseline is passed/failed" - Created an abstract Spring bean to propagate the configuration property to all visual testing step classes
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vividus-extension-visual-testing/src/main/java/org/vividus/visual/steps/AbstractVisualSteps.java | Implements the extended assertion format logic with conditional branching based on the enabled flag |
| vividus-extension-visual-testing/src/main/resources/vividus-extension/spring.xml | Defines abstract parent bean to configure the extended assertion format property |
| vividus-extension-visual-testing/src/main/resources/properties/defaults/default.properties | Sets the default value to false for backward compatibility |
| vividus-extension-visual-testing/src/test/java/org/vividus/visual/steps/VisualStepsTests.java | Adds test coverage for passed and failed scenarios with extended format |
| vividus-plugin-visual/src/main/resources/vividus-plugin/spring.xml | Updates VisualSteps bean to inherit from abstractVisualSteps parent |
| vividus-plugin-applitools/src/main/resources/vividus-plugin/spring.xml | Updates VisualTestingSteps and UfgSteps beans to inherit from abstractVisualSteps parent |
| docs/modules/plugins/pages/plugin-visual.adoc | Documents the new configuration property with acceptable values and description |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| verify(softAssert).recordFailedAssertion(EXTENDED_MESSAGE_FORMAT.formatted(BASELINE_NAME, "failed")); | ||
| } | ||
|
|
There was a problem hiding this comment.
The new test methods shouldUseExtendedAssertionFormatPassed and shouldUseExtendedAssertionFormatFailed only test the COMPARE_AGAINST action type. However, the verifyResult method has special logic for CHECK_INEQUALITY_AGAINST (line 94: XOR operation that inverts the result). The extended format should also be tested with CHECK_INEQUALITY_AGAINST to ensure the message correctly reflects "passed" or "failed" when the inequality check logic is applied. Consider adding tests for CHECK_INEQUALITY_AGAINST with both isPassed true and false scenarios.
| @Test | |
| void shouldUseExtendedAssertionFormatPassedForInequalityCheck() | |
| { | |
| var visualCheck = new AbstractVisualCheck(BASELINE_NAME, VisualActionType.CHECK_INEQUALITY_AGAINST) { }; | |
| var visualCheckResult = new VisualCheckResult(visualCheck); | |
| // For CHECK_INEQUALITY_AGAINST the verifyResult logic inverts the result, | |
| // so isPassed(false) should be treated as a passed assertion. | |
| visualCheckResult.setPassed(false); | |
| visualSteps.setExtendedAssertionFormatEnabled(true); | |
| visualSteps.verifyResult(visualCheckResult); | |
| verify(softAssert).recordPassedAssertion(EXTENDED_MESSAGE_FORMAT.formatted(BASELINE_NAME, "passed")); | |
| } | |
| @Test | |
| void shouldUseExtendedAssertionFormatFailedForInequalityCheck() | |
| { | |
| var visualCheck = new AbstractVisualCheck(BASELINE_NAME, VisualActionType.CHECK_INEQUALITY_AGAINST) { }; | |
| var visualCheckResult = new VisualCheckResult(visualCheck); | |
| // For CHECK_INEQUALITY_AGAINST the verifyResult logic inverts the result, | |
| // so isPassed(true) should be treated as a failed assertion. | |
| visualCheckResult.setPassed(true); | |
| visualSteps.setExtendedAssertionFormatEnabled(true); | |
| visualSteps.verifyResult(visualCheckResult); | |
| verify(softAssert).recordFailedAssertion(EXTENDED_MESSAGE_FORMAT.formatted(BASELINE_NAME, "failed")); | |
| } |
|
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests