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

feat(@spartacus/storefront): Adding different viewports to e2e testing (GH-1321) #1615

Merged
merged 34 commits into from
Mar 22, 2019

Conversation

bgambocjaviniar
Copy link
Contributor

@bgambocjaviniar bgambocjaviniar commented Mar 11, 2019

Closes #1321

Some mobile viewports for an e2e test does not exist at it was not necessary since it does not interact with mobile features, such as the hamburger, product search, and filter.

@marlass
Copy link
Contributor

marlass commented Mar 12, 2019

Tests should be placed only in smoke/regression directories, not in integration directory directly. It was changed a few days ago.

Copy link
Contributor

@marlass marlass left a comment

Choose a reason for hiding this comment

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

Avoid using should('exist') assertion. cy.get would fail if an element does not exist. No need to double check that. Look also at my other comment about files location.

@bgambocjaviniar
Copy link
Contributor Author

bgambocjaviniar commented Mar 12, 2019

Avoid using should('exist') assertion. cy.get would fail if an element does not exist. No need to double check that. Look also at my other comment about files location.

I agree with the file location. Very sorry! I forgot to move it, I was supposed to ask where I should put them yesterday, but for the assertion I kind of disagree.
https://docs.cypress.io/guides/core-concepts/introduction-to-cypress.html#Default-Assertions

Here it says we don't need to write .should('exist') as cy.get already does that, and that if we do chain any .should() command, it would no longer do the default assertion. I find that its better to explicitly say it other than implicitly because its more verbose and readable. For example, when a new developer starts working on cypress looks at cy.get('xx'), they probably would not know what it does unless they read the documentation, however, when they do cy.get('xx').should('exist'), they would understand it as it needs to exists in the dom

If you chain any .should() command, the default .should('exist') is not asserted.
^ From the link above

What do you think?

Copy link
Contributor

@KateChuen KateChuen left a comment

Choose a reason for hiding this comment

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

Move all mobile tests to a subfolder 'mobile' under regression.

@bgambocjaviniar
Copy link
Contributor Author

Storefinder was not added for e2e tests because there will be a refactor on it #1510, #1372, #1469 , #1470

@bgambocjaviniar bgambocjaviniar dismissed marlass’s stale review March 21, 2019 03:50

discussed with the team in Montreal

@KateChuen KateChuen merged commit 586f8ab into develop Mar 22, 2019
@kacperknapik kacperknapik deleted the feature/GH-1321 branch August 2, 2019 08:11
@bgambocjaviniar bgambocjaviniar restored the feature/GH-1321 branch September 9, 2020 14:59
@bgambocjaviniar bgambocjaviniar deleted the feature/GH-1321 branch January 25, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants