-
Notifications
You must be signed in to change notification settings - Fork 26
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
Load docker-compose configuration from smart and conventional locatio… #64
base: develop
Are you sure you want to change the base?
Load docker-compose configuration from smart and conventional locatio… #64
Conversation
734c7dc
to
f1851d9
Compare
|
Hey @Toilal I'm pretty hesitant to merge breaking changes. There are however a couple of really nice things in your PR that I want to merge. Expanding tests to more python versions and adding better errors are both awesome. As is changing the default away from I think my main problem with a large breaking change is that supporting multiple projects in one pytest run is quite easily solved with tox as well. If a user really needs this functionality it is quite straightforward to accomplish by marking the tests in a directory and calling pytest multiple times with the correct marker and docker compose file configured. For example, with this directory structure:
If the tests in foo&bar 1 are all marked
I assume that this is how people are doing it now and since it's currently possible with little effort I really don't want to do breaking changes to make it slightly more convenient. @todofixthis what are your thoughts on the matter? |
Actually since pytest allows running only tests from a specific directory users don't even need to bother with marks. They can simple run:
|
🤔 I'll admit, this all goes far beyond any use case I've ever encountered (let alone what I thought docker-compose was capable of 😅), so I can't speak to best practice WRT supporting multiple projects. But, one thing does stand out to me: As the logic for resolving docker-compose configuration gets more complex, it introduces ways for users to get unexpected/confusing results from their test runs. My preference would be to make configuration as dead-simple as possible, and to introduce complexity only where it saves the user from having to implement even more complex workarounds. Given this, I'm inclined to agree with @RmStorm's recommendation WRT the breaking change. I do think this is an interesting use case, though, so even if it's not explicitly supported by the plugin (i.e., the user would need to run multiple pytest commands to achieve the desired result), I think it would be good to explain it in our documentation. |
Hi, thanks for reviewing this one. First of all, we agree that some changes here are breaking, and this should be clearly documented. Next version should be a new major (4.0.0). I was using this module since about 6 month in a single project and with a single docker-compose configuration, everything was right. But problems started to arise when my project grows and I needed to add other docker-compose configuration files. I also encountered some problems when running a single test inside Pycharm, as it sets the current working directory in the directory of the test. pytest-docker-compose in this case doesn't find docker-compose.yml configuration and fails to run. It also seems counter-intuitive to run pytest many times to get all tests running. It just didn't came to me as a solution until I find the tox configuration file here. It seems more like a workaround than an actual way of thing should work. Another thing I would expect from the plugin is the ability to setup the configuration file right from the test, maybe with a new def configurable_test(docker_compose_configuration):
container_getter = docker_compose_configuration.get('./some/dir/my_docker_compose.yml', './another/override.yml')
container = container_getter.get('my_service') If such feature is implemented on top of this PR branch, the docker-compose project will be reused through the whole pytest session. About breaking changes in configuration: In fact, it won't break anything for most users, those using a single docker-compose configuration file. The loaded docker-compose configuration in this case will still be the same. This may break by loading another configuration only when user has many Users can also bring back the old behavior by adding the new To mitigate the removal of |
f061853
to
a3e2063
Compare
I have rebased this PR on develop and added Python 3.9 support in tox tests. |
c0dfc04
to
8ab98e6
Compare
…ns inside test directory structure. docker-compose configuration is now loaded from 3 directories: - Directory containing the actual test - "docker-compose" directory at the same level of the directory containing the actual test - Current working directory. Effective directory is the first one in this list where docker-compose can run properly. Effective directory can be forced with --docker-compose-directory option. All docker_project objects are now registered in all_docker_projects registry, a simple dict available as a session scoped fixture that keeps all docker_project instances involved in the pytest session. At the same time, docker_project fixture has been dropped. This change makes possible to have different docker_project instances for the run session, while still allowing to reuse the docker_project instance when the same configuration is involved. ContainerGetter has been improved to wait for container to be in running state before returning. To retrieve a stopped service, user can use set wait_running argument to False. This changes makes tests more stable. (In my use case, network_info was often empty because it was retrieved when a container was still starting or restarting) Some broken tests were fixed too. I also tried to update CircleCI configuration to make it test with Python 3.7/3.8. Close pytest-docker-compose#43 Close pytest-docker-compose#63 BREAKING CHANGE: --docker-compose option doesn't support "," separator anymore and should now be used many times to include many configuration files. BREAKING CHANGE: docker_project fixture has been dropped, use all_docker_projects instead.
8ab98e6
to
04f684a
Compare
@RmStorm would you consider merging this pull request ? |
Hey @Toilal I'm sorry for having been slow to respond. I've been very busy lately. My original opinion still stands:
However @vlagorsse came up with a minimal approach in #77 that I think will allow for solving #43 without a massive refactoring. So in order to proceed I propose the following. This PR has to be split into multiple smaller ones to make merging feasible in a safe manner. And any possible solution to #43 has to be implemented in such a way that doesn't require bumping the major version of this package by introducing breaking changes. |
…ns inside test directory structure.
First of all, sorry about the single big commit. But I think it will improve this plugin greatly
If merged, you should probably increment the major version
docker-compose configuration is now loaded from 3 directories:
Effective directory is the first one in this list where docker-compose can run properly. Effective directory can be forced with --docker-compose-directory option.
All
docker_project
objects are now registered inall_docker_projects
registry, a simple dict available as a session scoped fixture that keeps alldocker_project
instances involved in the pytest session. At the same time,docker_project
session scoped fixture has been dropped. This change makes possible to have differentdocker_project
instances for the run session, while still allowing to reuse thedocker_project
instance when the same configuration is involved.ContainerGetter has been improved to wait for container to be in running state before returning. To retrieve a stopped service, user can use set
wait_running
argument toFalse
. This changes makes tests more stable. (In my use case, network_info was often empty because it was retrieved when a container was still starting or restarting)Some broken tests were fixed too.
I also tried to update CircleCI configuration to make it test with Python 3.7/3.8.
Close #43
Close #63
BREAKING CHANGE:
--docker-compose
option doesn't support "," separator anymore and should now be used many times to include many configuration files.BREAKING CHANGE:
docker_project
fixture has been dropped, useall_docker_projects
instead.