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

Add a PHPUnit Docker Container #4279

Merged
merged 12 commits into from Jan 12, 2018

Conversation

Projects
None yet
3 participants
@pento
Member

pento commented Jan 4, 2018

While it's easy to set up the local testing environment for JS, it's fiddly and annoying to set up a similar environment for PHP.

This PR adds a PHPUnit docker container, so that tests can more easily be run. It also includes npm run shortcuts for running the tests.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 4, 2018

Member

This is cool, thanks for working on it!

The existing docker container has fully working WordPress installation. I don’t know what the good practices about Docker are, but in theory we could setup everything inside one container. Question is if we want to keep them separate?

Another question is if we want to update Travis configuration to use newly added JS run scripts wrappers? The benefit of doing so would be the feedback from Travis if this setup works properly as project progresses.

Member

gziolo commented Jan 4, 2018

This is cool, thanks for working on it!

The existing docker container has fully working WordPress installation. I don’t know what the good practices about Docker are, but in theory we could setup everything inside one container. Question is if we want to keep them separate?

Another question is if we want to update Travis configuration to use newly added JS run scripts wrappers? The benefit of doing so would be the feedback from Travis if this setup works properly as project progresses.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 4, 2018

Contributor

Yeah, that's great I always struggle running these tests. I have the same question as @gziolo Can't we use the default docker setup to run the unit tests there or do we want them to be isolated?

Contributor

youknowriad commented Jan 4, 2018

Yeah, that's great I always struggle running these tests. I have the same question as @gziolo Can't we use the default docker setup to run the unit tests there or do we want them to be isolated?

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jan 5, 2018

Member

I'm not particularly familiar with configuring Docker, so I found that using a pre-existing container was the easiest way to get it setup.

I think the PHPUnit behaviour could be merged into the WordPress container with a Dockerfile that installs all the necessary bits. I'll experiment a bit, and see how it works.

Member

pento commented Jan 5, 2018

I'm not particularly familiar with configuring Docker, so I found that using a pre-existing container was the easiest way to get it setup.

I think the PHPUnit behaviour could be merged into the WordPress container with a Dockerfile that installs all the necessary bits. I'll experiment a bit, and see how it works.

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jan 8, 2018

Member

Okay, this is looking pretty good, I think.

@gziolo, @youknowriad: What do you think?

Member

pento commented Jan 8, 2018

Okay, this is looking pretty good, I think.

@gziolo, @youknowriad: What do you think?

- stage: test
script:
- npm install || exit 1
- npm run ci || exit 1
- stage: test
php: 7.1
env: WP_VERSION=latest
env: WP_VERSION=latest DOCKER=true

This comment has been minimized.

@gziolo

gziolo Jan 8, 2018

Member

So unit tests and linting are now consolidated, nice 👍

@gziolo

gziolo Jan 8, 2018

Member

So unit tests and linting are now consolidated, nice 👍

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 8, 2018

Member

Travis is happy about the changes introduced. I will give it a try today. Code wise I don't have much to comment, as I don't have enough Docker experience :)

Member

gziolo commented Jan 8, 2018

Travis is happy about the changes introduced. I will give it a try today. Code wise I don't have much to comment, as I don't have enough Docker experience :)

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jan 8, 2018

Member

I pretty much learned Docker... today. So, you can catch up pretty quick. 😉

Member

pento commented Jan 8, 2018

I pretty much learned Docker... today. So, you can catch up pretty quick. 😉

@pento pento referenced this pull request Jan 10, 2018

Merged

Added class information for core/embed in the frontend #4118

3 of 3 tasks complete
@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jan 10, 2018

Member

@WordPress/gutenberg-core: I would like your opinion on 2460ba1. The other npm scripts added in this PR are new, so I'm okay with them requiring Docker. Changing the fixtures:server-registered script to use Docker pushes the development workflow heavily in the direction of requiring Docker.

It seems to me like Docker is a pretty good option for a workflow that isn't awful to setup, but it does mean becoming more opinionated about the development environment. Working from a custom WordPress install will be trickier, for example.

Member

pento commented Jan 10, 2018

@WordPress/gutenberg-core: I would like your opinion on 2460ba1. The other npm scripts added in this PR are new, so I'm okay with them requiring Docker. Changing the fixtures:server-registered script to use Docker pushes the development workflow heavily in the direction of requiring Docker.

It seems to me like Docker is a pretty good option for a workflow that isn't awful to setup, but it does mean becoming more opinionated about the development environment. Working from a custom WordPress install will be trickier, for example.

"ci": "concurrently \"npm run lint && npm run build\" \"npm run test-unit:coverage-ci\"",
"fixtures:clean": "rimraf \"blocks/test/fixtures/*.+(json|serialized.html)\"",
"fixtures:server-registered": "./bin/get-server-blocks.php > blocks/test/server-registered.json",
"fixtures:server-registered": "docker-compose -f docker/docker-compose.yml run -w /var/www/html/wp-content/plugins/gutenberg --rm wordpress ./bin/get-server-blocks.php > blocks/test/server-registered.json",

This comment has been minimized.

@youknowriad

youknowriad Jan 10, 2018

Contributor

cc @aduth as I think you added this script, which now requires docker

@youknowriad

youknowriad Jan 10, 2018

Contributor

cc @aduth as I think you added this script, which now requires docker

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jan 11, 2018

Member

Rebased on master.

Member

pento commented Jan 11, 2018

Rebased on master.

@pento pento merged commit 9483952 into master Jan 12, 2018

3 checks passed

codecov/project 39.78% remains the same compared to 02e6e0f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@pento pento deleted the add/phpunit-docker branch Jan 12, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 12, 2018

Member

Nice, now I can test on master. I was going to propose merging it by Monday anyway 😃

Member

gziolo commented Jan 12, 2018

Nice, now I can test on master. I was going to propose merging it by Monday anyway 😃

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jan 12, 2018

Member

Testing on master is the only place you can get accurate results.

Member

pento commented Jan 12, 2018

Testing on master is the only place you can get accurate results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment