chore: Safer visual tests for stories with `<img>`

r100-stack commented 1 year ago

Issue

I was on the iTwinUI-react GitHub page and saw that the CI is failing.

I think it is from the latest merged PR #974 in which the Avatar test is failing in this CI action.

Fix

Avatar's WithImage story is an Avatar with an with an src that needs to download the image.

The baseline image so far was actually an empty circle:

OLD METHOD

Adding a cy.wait(2000) actually downloads the image and displays/renders it. So now the new test image generated is:

NEW METHOD

Instead of cy.wait(2000), I am now using hide() to hide the <img> tags in all stories with <img>. I got this idea from Tile.test.ts L25-31. I used more specific selectors whenever possible (e.g. img.iui-icon) and otherwise used a normal img selector.

I also applied this fix for all stories with <img>.

This is a quick PR with this small fix so that CI does not fail. Hopefully we can merge it soon. Later if there is a better fix, we can do that.

Checklist

~[ ]~ Add meaningful unit tests for your component (verify that all lines are covered)
[x] Verify that all existing tests pass
~[ ]~ Add component features demo in Storybook (different stories)
[x] Approve test images for new stories
~[ ]~ Add screenshots of the key elements of the component

r100-stack commented 1 year ago

I think that the test for combobox loading state is unecessary if they end up just looking loaded

I think it would be safer to ignore the <img> there too. Because if for some reason the <img> doesn't load later, the test would fail.

Actually, the test failure mentioned in the PR description might have been because the baseline image was "without" loading the <img> but the test image on GitHub was "with" loading with the <img>.

Thus, I feel it is safer to ignore all <img>s with urls as their src.

elephantcatdog commented 1 year ago

I think it would be safer to ignore the there too. Because if for some reason the doesn't load later, the test would fail.

Actually, the test failure mentioned in the PR description might have been because the baseline image was "without" loading the but the test image on GitHub was "with" loading with the .

Thus, I feel it is safer to ignore all s with urls as their src.

I meant more like - don't add the test if it doesn't add value.

elephantcatdog commented 1 year ago

Other than that, it looks good.

elephantcatdog commented 1 year ago

I approved it; don't know if we should wait for an approval from someone else

iTwin / iTwinUI-react