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

Cypress multiple wishlist #3218

Merged
merged 33 commits into from
Jun 25, 2021
Merged

Cypress multiple wishlist #3218

merged 33 commits into from
Jun 25, 2021

Conversation

dpatil-magento
Copy link
Contributor

@dpatil-magento dpatil-magento commented Jun 4, 2021

Description

Cypress test for multiple wishlist

Related Issue

Closes PWA-1808

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Clone this branch and cd venia-integration-tests/
  2. yarn install
  3. yarn test
  4. From cypress UI run both wishlist E2E and integration tests and make sure they pass.

Screenshots / Screen Captures (if appropriate)

Breaking Changes (if any)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jun 4, 2021

Fails
🚫 A version label is required. A maintainer must add one.
Messages
📖

Associated JIRA tickets: PWA-1808.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 74bc078

@dpatil-magento dpatil-magento changed the title DRAFT - Cypress multiple wishlist Cypress multiple wishlist Jun 7, 2021
@supernova-at
Copy link
Contributor

@dpatil-magento I'm getting the following error attempting to run these tests:

Screen Shot 2021-06-14 at 3 04 30 PM

accountPassword
);

assertCreateAccount(firstName);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer only to assert the things that this specific case is testing for.

If creating an account doesn't work, that should show up as a failure within the create account section of tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, good call. I put it here to make sure cypress waits for account creation is complete before it goes to next step. that is visiting /wishlist route in this case. I have added comment above it, but let me know if you think of any better approach here.

Comment on lines +89 to +107
cy.intercept('POST', hitGraphqlPath, req => {
if (req.body.operationName.includes('createWishlist')) {
req.reply({
fixture:
'wishlist/multipleWishlist/wishlistPageCreateWishlist1.json'
});
}
}).as('createWishlist1');
cy.intercept('GET', getCustomerWishlistCall, {
fixture: 'wishlist/multipleWishlist/oneWishlistNoProductsPage.json'
}).as('getCustomerWishlist1');
createWishlist('Test List1');
cy.wait(['@getCustomerWishlist1']).its('response.body');
cy.wait('@createWishlist1').should(result => {
expect(result.request.body.variables).to.eql({
input: { name: 'Test List1', visibility: 'PRIVATE' }
});
expect(result.response.body).to.exist;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is described as "being able to add and remove products from wishlist", but this block of code creates an empty wishlist and asserts things about it (name, visibility).

It's fine if we have to create a wishlist as part of "being able to add and remove products from wishlist", but I would expect that to be included in something like the createWishlist helper function.

If we're asserting that a wishlist is created properly I'd expect that to happen in a separate test case focused specifically on that purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do realize I'm coming from a Unit Testing frame of mind here, so if integration tests are intended to "test everything about wishlists" in one test, we should at least name them as such and add comments for the different blocks / ideas / areas we're testing:

it('wishlist page should work', () => {
  // we can create a wishlist
  // ... relevant code and assertions ...
  
  // we can add products to a wishlist
  // ... relevant code and assertions ...

   // we can remove products to a wishlist
  // ... relevant code and assertions ...
  
  // etc.
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for sure I can add comments to clarify. It's always good to keep test simple and short when it comes to mocked data tests. But in this case, I was unable to mock account creation and to test wishlist we have to be within active session. So, to avoid account creation for every small of piece of functionality I have combined all in one.

venia-integration-tests/src/assertions/wishlist/index.js Outdated Show resolved Hide resolved
"data": {
"createWishlist": {
"wishlist": {
"id": "32",
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't actually get created on the backend so these ids don't need to constantly be updated, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it does not needed. As soon as page reloads or test is complete the data is wiped.

import { productPageSelectedWishlistButton } from '../../fields/productPage';

/**
* Utility function to add product to wishlist from product page
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this comment is correct. This asserts that the product's wishlist indicator button is filled in / exists, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, copy/paste error :P


export const productAugustaEarningsUrl = '/augusta-earrings.html';

export const productAugustaEarningsName = 'Augusta Earrings';
Copy link
Contributor

Choose a reason for hiding this comment

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

We might also consider a data structure like

const productAugustaEarrings = {
  name: 'Augusta Earrings`,
  url: '/augusta-earrings.html`
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated :)

venia-integration-tests/src/fixtures/productPage/index.js Outdated Show resolved Hide resolved
venia-integration-tests/src/fixtures/productPage/index.js Outdated Show resolved Hide resolved
Co-authored-by: Andy Terranova <13182778+supernova-at@users.noreply.github.com>
supernova-at
supernova-at previously approved these changes Jun 16, 2021
Copy link
Contributor

@supernova-at supernova-at left a comment

Choose a reason for hiding this comment

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

✅ Code approved
✅ Verification tests pass

larsroettig
larsroettig previously approved these changes Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants