Skip to content

test(env): replace custom scripts with pytest and docker-compose #1178

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

Merged
merged 4 commits into from
Sep 16, 2020

Conversation

nejch
Copy link
Member

@nejch nejch commented Sep 5, 2020

I like writing functional/integration tests because the GitLab API keeps changing so it's the only way to really check the responses are what is assumed in unit test fixtures. But the custom bash scripting in the environment was kind of unreliable with the REUSE_CONTAINER option, the custom venv creation, hardcoded temp dirs (I'm sometimes on a Windows machine 😢 ). It was also not a consistent experience compared to unit tests, so might be why people don't usually add them.

I saw that gitbeaker was doing something similar so I'm hoping this is a stable approach.

The idea was to:

  • have all virtualenvs fully managed by tox for consistency
  • fully cross-platform I hope (even tested on Windows cmd 😁 )
  • use native docker-compose, making it easy to add real integration tests with other services like gitlab-runner, LDAP etc.
  • create a static token directly with the rails runner, so no need for requests-html dependency and generate_token.py
  • remove all unneeded/wrapper bash scripts when it's all put together, use pytest libraries that can be more robust instead
  • easier to rewrite python_test_v4.py into real test cases that run independently (though it will make the runs longer I assume)
  • can use pytest's tmpdir fixtures later which might be more reliable than /tmp

Cons/TODOs before merge:

  • performance - not sure if it's just me but seems like the container takes a bit longer to spin up.
  • more pytest dependencies to install - I've added a separate docker-requirements.txt to avoid polluting other environments, does that make sense?
  • still need to implement a "reuse existing container" option upstream on pytest-docker. I have something working locally with pytest_addoption and there is some work already upstream in Provide fixture for docker cleanup command avast/pytest-docker#51

It's also possible this is a bit overkill 😁 but would love any feedback on this if anyone else was also having issues with the bash scripts before.

@codecov-commenter
Copy link

Codecov Report

Merging #1178 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1178   +/-   ##
=======================================
  Coverage   77.37%   77.37%           
=======================================
  Files          11       11           
  Lines        2816     2816           
=======================================
  Hits         2179     2179           
  Misses        637      637           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d662ab...8eab0d5. Read the comment docs.

Copy link
Member

@max-wittig max-wittig left a comment

Choose a reason for hiding this comment

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

Very very nice!

@nejch nejch marked this pull request as ready for review September 14, 2020 05:02
Copy link
Member

@max-wittig max-wittig left a comment

Choose a reason for hiding this comment

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

I'm really a fan of this, just two small comments.

@max-wittig
Copy link
Member

@nejch Sadly some conflicts happened. Could you please rebase?

@nejch
Copy link
Member Author

nejch commented Sep 16, 2020

@nejch Sadly some conflicts happened. Could you please rebase?

I've rebased and refactored those paths with helper fixtures where possible so it's hopefully more readable :)

@nejch nejch requested a review from max-wittig September 16, 2020 05:01
@max-wittig max-wittig merged commit 266030a into master Sep 16, 2020
@nejch nejch deleted the test/cleanup-env branch September 16, 2020 07:53
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.

3 participants