ataylorme / Advanced-WordPress-on-Pantheon

MIT License
65 stars 32 forks source link

Introduce basic unit tests for #30 #45

Closed tobeycodes closed 7 years ago

tobeycodes commented 7 years ago

This is very basic but works

I suggest:

In addition I think we should be able to easily run tests locally

ataylorme commented 7 years ago

Multidev created successfully! http://pr-45-pantheon-wp-best-practices.pantheonsite.io/

tobeycodes commented 7 years ago

Also why do we need to run tests on the test environment? Why don't we just do them on both. Because although this worked locally I can't test unless we merge into master which is probably not a great workflow

ataylorme commented 7 years ago

Also why do we need to run tests on the test environment? Why don't we just do them on both.

@schrapel my idea for tests/workflow is to run unit tests on every code push to this repo. Once pull requests are created a multidev is spun up on Pantheon for review as well. When the PR is merged master is deployed to the dev environment on Pantheon.

Then once multiple PRs/branches are merged into master on Pantheon they can be tested together in the test environment with Behat tests running on Pantheon. A Behat test suite will take longer to run so only running it once at the end of a sprint, new version, etc. makes the most sense to me.

It might not be ideal for this example repository but that's the workflow I would want to use on an in-development project. Lots of quick iterations with Pull requests, code reviews and basic unit tests to make sure the new code isn't breaking existing functionality. At the end of the code cycle take everything that's been merged into master and deploy to the test environment on Pantheon where Behat, load testing, and a more thorough QA can be done before deploying to production.

All that being said I do agree the tests should be able to be run locally but for Behat it can be called directly locally for testing.

tobeycodes commented 7 years ago

👍 That's all good. It's also why I didn't really do too much with this pull request because I wasn't sure the direction you wanted to go in. It's a start and will make more progress with it when I have more time

ataylorme commented 7 years ago

Rename $RUN_BEHAT_BUILD to $RUN_TESTS

I still like the idea of running unit tests on CircleCI and Behat on the Pantheon URL directly as Behat is testing the entire site, not just one piece of functionality, I think it's important to do it on infrastructure that mirrors production.

It's a start and will make more progress with it when I have more time

Cool. What else do you want to do in this PR? I'll have some time to work on it as well this week

tobeycodes commented 7 years ago

It would've been to just meet your requirements above. If you've got time I'll leave it to you :)

ataylorme commented 7 years ago

@schrapel I updated it so unit tests only run on master or a pull request. That way folks can iterate quickly on a branch without waiting for a slower build. Then when a PR is made and a branch is ready for review the unit tests will run.

Thoughts?

tobeycodes commented 7 years ago

I like it. But it does mean we can merge changes into master where tests are failing which probably isn't a good idea

ataylorme commented 7 years ago

But it does mean we can merge changes into master where tests are failing which probably isn't a good idea

An arbitrary branch could be created and merged to master on the command line. I set the master branch as protected but I'm not sure if that rejects local pushes or just requires review, etc. on a pull request.

ataylorme commented 7 years ago

@stevector what are your thoughts on:

1) Optimizing for build speed and only running unit tests on master on when Circle CI is fired via Quicksilver from a deployment to the test environment on Pantheon

vs.

2) Always running unit tests

Toby brings up a good point above about being able to merge a branch without passing tests. I'm leaning towards option 1, thinking GitHub's protected branch on master is sufficient. An admin can still override and merge items without passing tests - either locally or on GitHub - and don't think that's something we can truly enforce without self hosting git and having hooks reject merges if tests fail.

ataylorme commented 7 years ago

After chatting with @stevector it makes sense to always run unit tests. If they are true unit tests the speed difference will be negligible.