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: add cypress tests for playground and template apps #427

Merged
merged 3 commits into from
Mar 6, 2019

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented Mar 5, 2019

This should help us prevent possible regressions (e.g. #425).

PS: I've created a new org/project/test-user and added the env variables on travis.

@emmenko
Copy link
Member Author

emmenko commented Mar 6, 2019

Hmm it seems that our strategy with "skipping" the Percy tests does not really work. I guess we just skip running it and do not require the status check.

});

Cypress.Commands.add('login', () => {
cy.visit('/logout');
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we define the cmd above this could be cy.logout() I suppose. I guess, if it works, the same applies maybe in other repos.

"npm": ">=5",
"yarn": "~1.3.0||>=1.5.0"
"yarn": ">=1.13"
Copy link
Contributor

Choose a reason for hiding this comment

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

It is interesting how one could argue this being a breaking change. 😀

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm why would you say it's a breaking change? You mean for contributors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. Anybody installing, not having that version, will at least get a warning. I am not saying it should but it is an interesting thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we can also look it the other way around: local and CI should use the same yarn version to avoid possible weird issues 😜

LOGIN_PASSWORD: process.env.CYPRESS_LOGIN_PASSWORD,
PROJECT_KEY: process.env.CYPRESS_PROJECT_KEY,
}),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Simple and good. Wondering if one day we can actually move shared commands etc from the mc-fe here also allowing custom app authors to easily write e2e tests. Even though it ia likely an „unimportant“ feature wish right now. Anyway, it would also force us to have the same/in-sync logic across repos. So that e.g these env vars are read the same on travis and circle no matter what repo.

These things btw is what I hoped would be nice tasks for the full time focus FLD role.

Copy link
Member Author

Choose a reason for hiding this comment

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

allowing custom app authors to easily write e2e tests

Yeah, I was also thinking of having a cypress setup in e.g. the example apps. Or have a plugin package that injects commands (@commercetools-frontend/cypress-mc-app).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. My head was thinking more of @commercetools-frontend/cypress-mc-app. Maybe create an issue for it so we do not forget and can give it to an FLD who has time to pick it up? As a side effect of that we „force“ to share and gain knowledge of these things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done: #429

@emmenko
Copy link
Member Author

emmenko commented Mar 6, 2019

One small side-effect is that the build now takes >15min to run 😅 (because we build 2 apps, run tests on each one of them, etc).

We can maybe look in parallelizing some of the jobs.

@emmenko emmenko merged commit c90b24f into master Mar 6, 2019
@emmenko emmenko deleted the nm-cypress-tests branch March 6, 2019 14:10
@tdeekens
Copy link
Contributor

tdeekens commented Mar 6, 2019

One small side-effect is that the build now takes >15min to run 😅 (because we build 2 apps, run tests on each one of them, etc).

I guess we should analyse where we spend time. TravisCI generally seems a bit slower than it has to at times. Maybe running both in parallel really is an easy gain to just cut time in half.

@emmenko
Copy link
Member Author

emmenko commented Mar 6, 2019

I looked into the travis build stages (https://docs.travis-ci.com/user/build-stages) but they don't share artifacts between stages, you have to upload the files on your own.

I think in this case circleci will be better for defining parallel workflows (in case we want to proceed with that).

@tdeekens
Copy link
Contributor

tdeekens commented Mar 6, 2019

Yeah I think (personal opinion) there is benefit in settling on one CI solution across repos for various reasons (familiarity etc) and pay the price for speed.

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.

None yet

2 participants