Add tests for ContrastChecker component. #3888
Conversation
BE-Webdesign
added
the
[Type] Automated Testing
BE-Webdesign
force-pushed
the
add/test/blocks/contrast-checker
branch
from
e7cf3e6 to
1107042
Compare
| expect( shallow( <ContrastChecker /> ).html() ).toBeNull(); | ||
| } ); | ||
|
|
||
| test( 'should render null when the colors meet AA WCAG guidelines.', () => { |
There was a problem hiding this comment.
This test case is not covering anything specific to ContrastChecker ? Should we assume TinyColor is tested separately?
There was a problem hiding this comment.
I think we assume that TinyColor is tested separately. If the colors are readable and expect that to be true, the component does not render anything. The component honestly confused me a lot, because I didn't fully realize what it did, but I think the intent of the component is to not do anything when the accessibility is fine for the colors. This test basically covers the most common use case.
| expect( componentWrapper ).toMatchSnapshot(); | ||
| } ); | ||
|
|
||
| test( 'should render different message matching snapshot when background color has less brightness than text color and different text size.', () => { |
There was a problem hiding this comment.
It looks like this part "and different text size." can be removed. As long as the text is not readable text size does not affect the message being shown.
| textColor={ sameShade } | ||
| isLargeText={ isLargeText } | ||
| fallbackBackgroundColor={ fallbackBackgroundColor } | ||
| fallbackTextColor={ fallbackTextColor } /> |
There was a problem hiding this comment.
Given we are not testing something related to fallback colors in this test case and in test case "should render different message matching snapshot when background color has less brightness than text color and different text size.", I think it would be better to not pass fallback colors in both of this cases. Then we can add a test that would make sure that if both normal colors and fall colors are passed the message shown is the result of the usage of normal colors.
There was a problem hiding this comment.
Hi @BE-Webdesign that you for providing tests for this
component 👍 I left some comments. Another point is that, In my option, we should not include the expects related to the tinycolor output. What we need to make sure is that for the props we pass to contrast checker depending if they pass AA test or what is darker background or text the correct message is shown or nothing is shown.
The fact the component uses tinycolor2 is something internal. But I don't have a strong option and I would like to know more inputs in this subject.
|
@jorgefilipecosta Thank you for the extra review and comments!
The only argument I have for leaving them in is to verify that tinycolor2 actually thinks the colors are valid values. If we don't assert that the colors meet AA, it could easily lead to testing false positives and negatives, because we would skip over what tinycolor2 actually thinks of the colors. |
BE-Webdesign
force-pushed
the
add/test/blocks/contrast-checker
branch
from
1107042 to
ed19788
Compare
|
|
|
Hi @BE-Webdesign, the tests look good to me, thank you for making the changes 👍 I would just add a test case that mixes non-compliant fallbackBackgroundColor and normal textColor, because it is a very common use case. |
BE-Webdesign
force-pushed
the
add/test/blocks/contrast-checker
branch
2 times, most recently
from
c5bd8e1 to
8197214
Compare
|
Okay added the extra test case for valid text color with invalid background fallback. |
jorgefilipecosta
force-pushed
the
add/test/blocks/contrast-checker
branch
from
8197214 to
0e7f9cd
Compare
There was a problem hiding this comment.
Tests were failing (not related to this changes) I rebased and they seem fine now. Thank you for addressing the feedback, the changes seem fine to me 👍
Edit: CI tests, broke because of a network problem if we can rebase again so tests are re-executed, it would be perfect so we can get a green light from our CI system.
Add tests for ContrastChecker component.