Skip to content

Add a PHPUnit Docker Container#4279

Merged
pento merged 12 commits intomasterfrom
add/phpunit-docker
Jan 12, 2018
Merged

Add a PHPUnit Docker Container#4279
pento merged 12 commits intomasterfrom
add/phpunit-docker

Conversation

@pento
Copy link
Copy Markdown
Member

@pento 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.

@pento pento added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Build Tooling Issues or PRs related to build tooling [Type] Enhancement A suggestion for improvement. labels Jan 4, 2018
@pento pento self-assigned this Jan 4, 2018
@pento pento requested a review from youknowriad January 4, 2018 05:20
@gziolo
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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?

@pento
Copy link
Copy Markdown
Member Author

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 pento force-pushed the add/phpunit-docker branch from bb1c132 to cef1b09 Compare January 8, 2018 03:07
@pento
Copy link
Copy Markdown
Member Author

pento commented Jan 8, 2018

Okay, this is looking pretty good, I think.

@gziolo, @youknowriad: What do you think?

- stage: test
php: 7.1
env: WP_VERSION=latest
env: WP_VERSION=latest DOCKER=true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So unit tests and linting are now consolidated, nice 👍

@gziolo
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member Author

pento commented Jan 8, 2018

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

@pento
Copy link
Copy Markdown
Member Author

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.

package.json Outdated
Copy link
Copy Markdown
Contributor

@youknowriad youknowriad Jan 10, 2018

Choose a reason for hiding this comment

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

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

@pento pento force-pushed the add/phpunit-docker branch from e384f3f to 6317ed2 Compare January 11, 2018 00:49
@pento
Copy link
Copy Markdown
Member Author

pento commented Jan 11, 2018

Rebased on master.

@pento pento merged commit 9483952 into master Jan 12, 2018
@pento pento deleted the add/phpunit-docker branch January 12, 2018 04:10
@gziolo
Copy link
Copy Markdown
Member

gziolo commented Jan 12, 2018

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

@pento
Copy link
Copy Markdown
Member Author

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

Labels

[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Build Tooling Issues or PRs related to build tooling [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants