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

Unified testing framework #212

Merged
merged 4 commits into from Jan 28, 2021
Merged

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Jan 28, 2021

Problem

See #210, installing mypy in the Dockerfile caused problems related to dependencies-of-dependencies that were installed at the system level and then not available on a fresh image with only the user install copied over.

Motivation

In the Discord discussion of the first iteration of this pull request, we agreed that it would be nice to have one simple command that developers could run to test everything, and to re-use that same command in our GitHub workflows. It looked like the test_suite option in setup.py, supporting the python setup.py test command, might be a step in that direction.

Enter pytest-dev/pytest#5534. They're deprecating python setup.py test.

In the Discord discussion of the second iteration of this pull request, tox was ruled out as a solution because we already use Docker to manage our virtual environments and would prefer to run our tests in the same environment where the actual code runs. So now pytest is winning.

Changes

  • Now we don't install or run mypy in the Dockerfile
  • A new test workflow is created to run all tests
  • The separate mypy and unittest workflows are deleted

For me, this is how to run tests, though if you use a distro that doesn't try to mix Python 2 and 3 like Ubuntu does, drop the -3:

cd netkan
pip install --user .[test]
pytest-3 --mypy -v

This provides a single uniform access point for all testing, which will work the same for developers and for CI/CD.

@HebaruSan HebaruSan added Environment It's about the dev/prod environment Tests Tests, we need more tests! In Progress labels Jan 28, 2021
@HebaruSan

This comment has been minimized.

@techman83
Copy link
Member

I guess unittest is not a package?

unittest is a part of python standard lib.

Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

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

I think we can drop mypy from the container, it's non-runtime type checking. But our tests should still be run inside the container.

Though we don't use pytest or any other test packages in the future, so we may want to bust out mypy into a separate dependency set.

.github/workflows/mypy.yml Outdated Show resolved Hide resolved
.github/workflows/unittest.yml Outdated Show resolved Hide resolved
netkan/Dockerfile Show resolved Hide resolved
netkan/setup.py Show resolved Hide resolved
@HebaruSan HebaruSan changed the title Install test deps only when needed Unified testing framework Jan 28, 2021
@HebaruSan HebaruSan force-pushed the fix/test-inst branch 2 times, most recently from a73e447 to 085c2eb Compare January 28, 2021 05:50
@HebaruSan

This comment has been minimized.

Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

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

Tox has its use case, but I feel it's a little redundant at best and at worst may be doing something we don't expect. We can already manage environments in our workflows and our container.

pytest is the go to for testing frameworks. It has wrappers for mypy, linting, flake8 etc. Which would allow us to close off #49

It is rather daft that the python setup.py test is now deprecated, but the python community have been re-inventing the packaging wheel for as long as I can remember.

@techman83
Copy link
Member

Ok, so I've put my code where my mouth is! 🙂

  • test stage for docker, which uses production as a base. These layers won't end up in production as we target production, but will test in the same environment.
  • keeping the --help check in production is still a good sanity check
  • pytest used in the test workflow
  • tests are run before the deployment is produced
  • vs code environment works correctly (this was a right pain..)

@HebaruSan

This comment has been minimized.

@HebaruSan
Copy link
Member Author

@techman83 your changes look good to me. Should we look into pylint and flake8?

@techman83
Copy link
Member

Yes, but I think a separate PR. I enabled pylint and got a whole suite of warnings I didn't want to deal with on a PR for unifying our testing.

@techman83 techman83 merged commit 442038a into KSP-CKAN:master Jan 28, 2021
@HebaruSan HebaruSan deleted the fix/test-inst branch January 28, 2021 23:50
@HebaruSan HebaruSan mentioned this pull request Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Environment It's about the dev/prod environment In Progress Tests Tests, we need more tests!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants