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

Docker to run tests locally #2175

Closed

Conversation

josemiguelq
Copy link
Contributor

@josemiguelq josemiguelq commented Mar 31, 2020

Q A
Type improvement
BC Break no
Fixed issues

Summary

In order to run test locally without Travis service and using the necessary dependencies with a local configuration.

@josemiguelq josemiguelq changed the title Docker to run tests Docker to run tests locally Apr 1, 2020
- mongodb

mongodb:
container_name: mongodb
Copy link
Contributor

Choose a reason for hiding this comment

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

You can safely skip declaring container_name. Without it, docker will create name from COMPOSE_PROJECT_NAME or current directory (in this order), and append _<service_name>_1 to it.

container_name: mongodb
image: mongo
ports:
- 27777:27017
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use expose instead. Container running tests will be able to access mongo database using mongodb:27017 (service name becomes it's network name in internal network namespace)

To run the test for this package, run:

```
docker-compose up
Copy link
Contributor

Choose a reason for hiding this comment

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

MongoDB will be left running in the background. You might want to either stop it (docker-compose stop mongodb), or destroy the container altogether (docker-compose down).


RUN pecl -q install -f mongodb-$DRIVER_VERSION && docker-php-ext-enable mongodb

COPY . /code
Copy link
Contributor

@Steveb-p Steveb-p Apr 1, 2020

Choose a reason for hiding this comment

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

If this container is supposed to be used only with /code as a mounted volume, then copying the code into it will simply make it unnecessarily larger.

And btw, without the need to copy application code into the container, you no longer need to expand docker context to root directory in docker-compose.yaml, as you do now.

Smaller docker context means docker will work a little faster since it won't need to receive as much data (context) when starting up the build.

ADAPTER_VERSION: "^1.0.0"
DRIVER_VERSION: "stable"
volumes:
- .:/code
Copy link
Contributor

Choose a reason for hiding this comment

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

For MacOS users it might be helpful to mark it as :cached. See https://docs.docker.com/docker-for-mac/osxfs-caching/

Or, if container should not propagate changes to host, you can mark it as :ro, as readonly.

    volumes:
      - .:/code:cached

@alcaeus
Copy link
Member

alcaeus commented Apr 1, 2020

Thanks for your contribution @josemiguelq! Before you continue this work, I'd like to point out that we've previously declined a docker contribution, since none of us use docker in their development workflow (see #1868 (comment) for a longer explanation).

That said, I'm ok with providing a docker setup, given that some basics are covered:

  • The setup needs to be tested: since none of the maintainers use docker in their workflow, we most likely wouldn't catch errors in the docker setup. This can lead to a bad experience for users if the "just run this command" setup does not work
  • By merging the pull request, the maintainers essentially agree that they will be maintaining the code provided by a contributor. Given what I said above, none of us currently feel comfortable to do so, so we're relying on the community to maintain this part. If the setup breaks our CI pipeline, this will most likely lead to the setup being removed to not block any work unless a contributor can fix it in a timely manner.

Unrelated to the above points, you've created you pull request against the 1.3.x branch. This branch is in bug fixing mode only, and is not worth any additional effort, especially given that 2.x uses a different architecture (it uses a different driver and doesn't need the mongo-php-adapter package), which would require the work to be repeated.

I'm closing the PR since it targets the wrong branch and invite you to re-create it against the master branch if you are ok with the points mentioned above.

@alcaeus alcaeus closed this Apr 1, 2020
@Steveb-p
Copy link
Contributor

Steveb-p commented Apr 1, 2020

@alcaeus in case of core Doctrine ODM team that holds true, but in case of small contributors - like myself - setting up a proper testing environment, with integration tests against a real database - proved to be rather difficult and undocumented.

This made it more difficult for me to join and properly test my code without running it on Travis, locally. In my opinion it might be beneficial to either provide what testing environment should look like. Using docker the tasks of setting up PHP and MongoDB are simplified.

While obviously maintaining this is out of scope for core, I would say it's worthwile if it's noted that it is a "best effort" approach.

This does not impact actual tests in any way. Lower barrier of entry to project might bring more benefit that introduced cost of maintenance.

@alcaeus
Copy link
Member

alcaeus commented Apr 1, 2020

@Steveb-p thank you for your insights.

In case the wording was not optimal, I'm not closing this PR because we're not supporting it, but because it was created against the wrong branch. If people need this setup and are willing to maintain it, I'll gladly merge a pull request.

At the same time, I won't be using docker for anything in the near future, as my current setup covers more use-cases (which admittedly is a side effect of working with MongoDB day-in, day-out), and is still painfully slow on MacOS on top of that. However, I don't want to force my way of doing things (TM) on anyone, just like I don't want to be forced someone else's way of doing things on me.

The above is the reason why I'd like this tested (somehow): this helps ensure that new users will find a working solution in docker if they need it, without being forced to use it if they don't want to. This doesn't mean that we should run the test suite on docker in travis-ci, but we should at least try to build the docker image to ensure it succeeds.

@josemiguelq
Copy link
Contributor Author

@alcaeus @Steveb-p
I'm glad for your review before I finish the PR.

I currently using MacOs and my first contribution was very painful because in my case this will mess my current local setup :/. I try keep the the environments already used in travis CI, but you dont need use for Travis, because my goal is the local setup.

I test the changes using docker to run the test of my last PR.

I agree with your points about none of you currently feel comfortable because you don’t use docker in development workflow, but the points of @Steveb-p are very interesting too and this is my the motivation, make easily the contribution.

@alcaeus What can we do to make you feel comfortable with a test of this Docker setup?

@josemiguelq
Copy link
Contributor Author

@alcaeus Can you reopen? I can’t reopen to change the base and I would like to keep the comments of this PR.

@alcaeus
Copy link
Member

alcaeus commented Apr 1, 2020

Can you reopen? I can’t reopen to change the base and I would like to keep the comments of this PR.

As much as I'd like to, GitHub won't let me:
image

Please create a new pull request and link it to this one. Sorry for the extra work.

What can we do to make you feel comfortable with a test of this Docker setup?

I think if we have a travis build step that builds the docker image and makes sure it's successful, this would be a good first step to ensure the docker setup is still working. We can then always improve on it or do more with the docker setup.

@Steveb-p
Copy link
Contributor

Steveb-p commented Apr 1, 2020

As much as I'd like to, GitHub won't let me

It should allow it if the josemiguelq:feature/docker-local-test branch is force pushed back to e30bb79. Then after reopening it can be force pushed again to the current position it is on.

@josemiguelq even if this commit no longer exists in your repository, as long as git garbage collector didn't run, it should be accessible to you by

git checkout e30bb79

Git usually keeps commits for some time before discarding them.

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.

None yet

3 participants