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

Load docker-compose configuration from smart and conventional locatio… #64

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Toilal
Copy link

@Toilal Toilal commented Sep 11, 2020

…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:

  • 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 session scoped 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 #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, use all_docker_projects instead.

@Toilal Toilal force-pushed the configuration-inside-tests branch 3 times, most recently from 734c7dc to f1851d9 Compare September 11, 2020 16:14
@Toilal
Copy link
Author

Toilal commented Sep 11, 2020

  py35-pytest3: commands succeeded
  py35-pytest4: commands succeeded
  py35-pytest5: commands succeeded
  py35-pytest6: commands succeeded
  py36-pytest3: commands succeeded
  py36-pytest4: commands succeeded
  py36-pytest5: commands succeeded
  py36-pytest6: commands succeeded
  py37-pytest3: commands succeeded
  py37-pytest4: commands succeeded
  py37-pytest5: commands succeeded
  py37-pytest6: commands succeeded
  py38-pytest3: commands succeeded
  py38-pytest4: commands succeeded
  py38-pytest5: commands succeeded
  py38-pytest6: commands succeeded
  congratulations :)

@RmStorm
Copy link
Contributor

RmStorm commented Sep 14, 2020

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 --file docker-compose.yml. These are excellent fixes/improvements that I definitely want to merge.

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:

|- it1/
    |- docker-compose.yml
    |- foo1_test.py
    |- bar1_test.py
|- it2/
    |- docker-compose.yml
    |- foo2_test.py
    |- bar2_test.py

If the tests in foo&bar 1 are all marked dir-it1 by adding @pytest.mark.dir-it1 and the ones in directory two likewise. All tests could be run by putting these two commands in tox or a bash file.

pytest -m dir-it1 --docker-compose=/it1/docker-compose.yml
pytest -m dir-it2 --docker-compose=/it2/docker-compose.yml

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?

@RmStorm
Copy link
Contributor

RmStorm commented Sep 14, 2020

Actually since pytest allows running only tests from a specific directory users don't even need to bother with marks. They can simple run:

pytest it1/ --docker-compose=/it1/docker-compose.yml
pytest it2/ --docker-compose=/it2/docker-compose.yml

@todofixthis
Copy link
Member

🤔 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.

@Toilal
Copy link
Author

Toilal commented Sep 15, 2020

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 docker_compose_configuration fixture.

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 docker-compose.yml configuration files inside it's test directory structure and --docker-compose option is not set.

Users can also bring back the old behavior by adding the new --docker-compose-directory . to pytest options (to quickly fix their tests).

To mitigate the removal of , split in --docker-compose option, I will bring it back and add a Deprecation warning when used, so it will be effectively removed in a later release.

@Toilal Toilal force-pushed the configuration-inside-tests branch 3 times, most recently from f061853 to a3e2063 Compare February 10, 2021 16:16
@Toilal
Copy link
Author

Toilal commented Feb 10, 2021

I have rebased this PR on develop and added Python 3.9 support in tox tests. --docker-compose split with , is still there with a deprecation warning.

@Toilal Toilal force-pushed the configuration-inside-tests branch 2 times, most recently from c0dfc04 to 8ab98e6 Compare February 10, 2021 16:25
…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.
@Toilal
Copy link
Author

Toilal commented Feb 15, 2021

@RmStorm would you consider merging this pull request ?

@RmStorm
Copy link
Contributor

RmStorm commented Feb 19, 2021

@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:

Expanding tests to more python versions and adding better errors are both awesome. As is changing the default away from --file docker-compose.yml. These are excellent fixes/improvements that I definitely want to merge.

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.

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.

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.

Traverse directories up to find docker-compose.yml file Multiple compose files
3 participants