Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(e2e): add toContainAStory assertion to visitStory #16132

Conversation

tay1orjones
Copy link
Member

This adds a new assertion to visitStory that checks to make sure an element with the .cds--layout class is present. The storybook config has a global decorator that applies this class via <Layout>

<Layout size={layoutSize || null} density={layoutDensity || null}>

This should ensure we avoid a bug where if you give visitStory invalid parameters (a wrong id, component, etc) the url won't point to a story but storybook still resolves it and gives their error screen:

image

Huge thanks to @matthewgallo for bringing this up!

Changelog

New

  • Add toContainAStory assertion to visitStory

Testing / Reviewing

  • You can view the error this produces by pulling this down locally, changing the name of a story in any .stories.js file, and running yarn playwright test

@tay1orjones tay1orjones requested a review from a team as a code owner April 4, 2024 03:00
@tay1orjones tay1orjones self-assigned this Apr 4, 2024
@tay1orjones tay1orjones changed the title test(playwright): add toContainAStory assertion to visitStory test(e2e): add toContainAStory assertion to visitStory Apr 4, 2024
Copy link

netlify bot commented Apr 4, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit a69d9da
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6650a1f4e11f9500083751d4
😎 Deploy Preview https://deploy-preview-16132--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tay1orjones
Copy link
Member Author

This fix has actually found 68 errors! It's why ci is failing. I'll work on fixing them

@tay1orjones tay1orjones marked this pull request as draft April 4, 2024 17:11
Copy link

netlify bot commented May 24, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit a69d9da
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6650a1f466f42e00081cd362
😎 Deploy Preview https://deploy-preview-16132--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tay1orjones
Copy link
Member Author

Looked at this today with @Gururajj77 and noticed that when an id of a story is updated to be correct, there's a high likelihood that we then see accessibility violations that need to be fixed.

Instead of fixing those accessibility errors in this PR, we should skip those tests to focus on getting this PR merged with the toContainAStory assertion to protect all the existing passing tests.

So the steps for this PR are now going to be:

  1. Correct the id for any test that fails the toContainAStory assertion
  2. If correcting that id results in new accessibility violations on that story, mark that test as test.skip for now
  3. Repeat steps 1 and 2, going through all the failures of the toContainAStory assertion
  4. Once all tests pass the toContainAStory assertion, make a new issue of all the tests that were test.skipped in this PR. We'll go fix those new violations one by one in follow up PRs.

@Gururajj77
Copy link
Contributor

opened new PR with changes, #16582

@Gururajj77 Gururajj77 closed this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants