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.

Sign up for GitHub

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

Jump to bottom

test(carousel): add testing for carousel default, with images, with videos, with media #8538

Merged
merged 12 commits into from
Mar 28, 2022
Merged

test(carousel): add testing for carousel default, with images, with videos, with media #8538

merged 12 commits into from
Mar 28, 2022

Conversation

mwanberg
Copy link
Contributor

Related Ticket(s)

[Test Scenario]: Carousel - (Default, with images, with videos, with Media)

Description

The original requirements can be found in the ticket. I erred a little on the verbose side, and gave each variant a full suite of tests applicable to its unique content. If I should pare back and if any are redundant, let me know.

Test scenario steps

Note: Accessibility and theme compliance tests were applied to all.

Carousel - Default

  1. Check the content (heading, copy, and arrow link) are appearing on the carousel cards
  2. Check the headers to make sure they all have the same height and are not set to 0px tall, providing an even vertical height for all the copy text
  3. Check that the whole card is clickable
  4. Ensure that the Next and Previous buttons move the carousel

Carousel - With Images

  1. Check the content (heading, copy, and arrow link) are appearing on the carousel cards
  2. Ensure that the images are rendered
  3. Check the headers to make sure they all have the same height and are not set to 0px tall, providing an even vertical height for all the copy text
  4. Check that the whole card is clickable
  5. Ensure that the Next and Previous buttons move the carousel

Carousel - With Videoes

  1. Check the content (heading, copy, and arrow link) are appearing on the carousel cards
  2. Ensure that the video thumbs and play buttons are rendered correctly
  3. Ensure that the duration text value is visible in the CTA area
  4. Check the headers to make sure they all have the same height and are not set to 0px tall, providing an even vertical height for all the copy text
  5. Check that the whole card is clickable
  6. Ensure that the Next and Previous buttons move the carousel

Carousel - With a mixture of images and videos (New SB Variant)

  1. Check the content (heading, copy, and arrow link) are appearing on the carousel cards
  2. Ensure that the images are rendered
  3. Ensure that the video thumbs and play buttons are rendered correctly
  4. Ensure that the duration text value is visible in the CTA area
  5. Check the headers to make sure they all have the same height and are not set to 0px tall, providing an even vertical height for all the copy text
  6. Check that the whole card is clickable
  7. Ensure that the Next and Previous buttons move the carousel

Changelog

New

  • Test for the Carousel component

@mwanberg mwanberg requested a review from a team as a code owner March 17, 2022 05:14
@mwanberg mwanberg requested review from RichKummer and annawen1 March 17, 2022 05:14
@mwanberg mwanberg added package: web components Work necessary for the IBM.com Library web components package test: e2e used for automated e2e testing 👀 eyes needed adopter: AEM used when component or pattern will be used by this adopter labels Mar 17, 2022
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Mar 17, 2022 •
edited
Loading

Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/8538/index.html

Built with commit: 43864be73df5efe1add6512a25f2f432510bd753

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Mar 17, 2022 •
edited
Loading

Deploy preview created for package NextJS Test Application:
https://ibmdotcom-nextjs-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8538/index.html

Built with commit: 43864be73df5efe1add6512a25f2f432510bd753

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Mar 17, 2022 •
edited
Loading

Deploy preview created for package Web Components (React wrapper):
https://ibmdotcom-web-components-react.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8538/index.html

Built with commit: 43864be73df5efe1add6512a25f2f432510bd753

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Mar 17, 2022 •
edited
Loading

Deploy preview created for package Web Components HTML Test Application:
https://ibmdotcom-web-components-html-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8538/index.html

Built with commit: 43864be73df5efe1add6512a25f2f432510bd753

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Mar 17, 2022 •
edited
Loading

Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8538/index.html

Built with commit: 43864be73df5efe1add6512a25f2f432510bd753

@andy-blum
Copy link
Member

Looks like it's just a formatting error remaining. yarn format should fix it. All the tests you wrote passed.

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Mar 17, 2022 •
edited
Loading

Deploy preview created for package "Web Components (Codesandbox Examples)":
https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8538/index.html

Built with commit: 43864be73df5efe1add6512a25f2f432510bd753

andy-blum
andy-blum requested changes Mar 17, 2022
View reviewed changes
Copy link
Member

@andy-blum andy-blum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a solid foundation here. Mainly just opinions and nitpicks in the comments. Let me know if you have questions/need more info!

packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated
* @param default - Path to default variant
* @param withImages - path to variant with images
* @param withCta - path to variant with videos
* @param withCta - path to variant with both images and videos
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work documenting the code, but lets make sure the @param names match the keys in the object

packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated
*/
const _paths = {
default: '/iframe.html?id=components-carousel--default',
withImages: 'iframe.html?id=components-carousel--cards-with-images',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests passed, so this might just be nitpicky, but can we make sure the paths all follow the same pattern (either start with leading slash or dont)

packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated
const insetValue = after.getPropertyValue('inset');

expect(positionValue).to.eq('absolute');
if (Cypress.browser.name !== 'firefox') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we excluding firefox here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, this code is a copy of very similar code elsewhere. Looking at the git history, it seems that was added in this PR: #7807 It seemed prudent to keep it in, though if it's now unneeded I can remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this for now until someone tells you to add it back in.

packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated

cy.get(_selectors.buttonPrevious).click();

cy.takeSnapshots();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think you ought to be able to chain the takeSnapshots into these and only need to call on cy once.

Not Nit: Does the scroll happen instantly or is it using smoothScroll behavior? If the scroll takes time, we might want to throw a .wait(1000) in here to make sure the page has time to scroll before we take snapshots

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points. I chained them and added a wait.

packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Show resolved Hide resolved
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Mar 17, 2022 •
edited
Loading

Deploy preview created for package "React (Codesandbox Examples)":
https://react-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8538/index.html

Built with commit: 43864be73df5efe1add6512a25f2f432510bd753

jkaeser
jkaeser suggested changes Mar 17, 2022
View reviewed changes
Copy link
Member

@jkaeser jkaeser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwanberg This is looking great! I left a few notes for your consideration. Mostly little nitpicks and clean ups, and a couple suggestions that should make the tests more robust.

packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated Show resolved Hide resolved
packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated Show resolved Hide resolved
@proeung
Copy link
Contributor

proeung commented Mar 23, 2022

@mwanberg Can you take a look at the feedback that @andy-blum and @jkaeser have given?

@mwanberg mwanberg force-pushed the test/8330-add-carousel-tests branch from d448f1a to a49b343 Compare March 24, 2022 05:17
@annawen1 annawen1 closed this Mar 25, 2022
@annawen1 annawen1 reopened this Mar 25, 2022
@proeung proeung added Ready to merge Label for the pull requests that are ready to merge and removed 👀 eyes needed labels Mar 25, 2022
kennylam
kennylam reviewed Mar 28, 2022
View reviewed changes
Copy link
Member

@kennylam kennylam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mwanberg! Just one small fix and it looks like it's good to go!

packages/web-components/tests/e2e-storybook/cypress/integration/carousel/carousel.e2e.js Outdated Show resolved Hide resolved
@mwanberg
Copy link
Contributor Author

Thanks @mwanberg! Just one small fix and it looks like it's good to go!

Done!

kennylam
kennylam approved these changes Mar 28, 2022
View reviewed changes
Copy link
Member

@kennylam kennylam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @mwanberg!

@kodiakhq kodiakhq bot merged commit 9299ca2 into carbon-design-system:main Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy-blum andy-blum andy-blum approved these changes

@jkaeser jkaeser jkaeser approved these changes

@kennylam kennylam kennylam approved these changes

@RichKummer RichKummer Awaiting requested review from RichKummer RichKummer is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@annawen1 annawen1 Awaiting requested review from annawen1 annawen1 is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@proeung proeung Awaiting requested review from proeung

@IgnacioBecerra IgnacioBecerra Awaiting requested review from IgnacioBecerra

Assignees
No one assigned
Labels
adopter: AEM used when component or pattern will be used by this adopter package: web components Work necessary for the IBM.com Library web components package Ready to merge Label for the pull requests that are ready to merge test: e2e used for automated e2e testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mwanberg @ibmdotcom-bot @andy-blum @proeung @kennylam @jkaeser @annawen1