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

Unittests, linting and code formatting using nox #28

Merged
merged 17 commits into from
May 9, 2020
Merged

Conversation

davidverweij
Copy link
Owner

@davidverweij davidverweij commented May 8, 2020

Closes #10 .

And works on point 1 and 2 of #23

What Changed (and why)?

I've followed guide 2 (Testing) and 3 (Linting) from Claudio Jolowicz's Hypermodern Python article to implement @jawrainey's suggested code linting, unittesting and pre-commit hooks. More specifically, I've implemented the following:

  1. Set up testing using pytest and coverage
  2. Wrote initial unit-tests for the CLI only at the moment using click.testing
  3. Set up automation for tests using nox, ensuring it uses pinned dependencies as declared in the poetry.lock to ensure testing amongst collaborates is identical. Note the install_with_constraints() wrapper in the noxfile.py as per Claudio Jolowicz's suggestion. Nox is now set up to test the script in --no-dev mode to reflect truer use, and not install all dev dependencies in each virtual environment.
  4. Expanded nox to run linting using flake8, code formatting using black and basic safety checks using safety. Currently, the setup includes flake8, flake8-bandit, flake8-black, flake8-bugbear, and flake8-import-order.
  5. Set up pre-commit hooks, and additionally runs black and flake8 per each commit. This currently only checks changed files.

Changed files

  1. pyproject.toml now includes dev dependencies for pytest (unit testing), coverage (unit test code coverage), black (auto code formatting), flake8 (linting and code-ordering)
  2. noxfile.py contains the setup for running nox
  3. tests/test_cli.py contains test cases for the CLI interface. Test can be expanded on by adding files in the same folder, such as a tests/test_module.py file.
  4. .flake8 contains settings for linting,
  5. .pre-commit-config.yaml contains setttings for the pre-commit hooks. Changes to this file need to be installed via pre-commit install in the root folder.
  6. Running linting resulted in changes to csv2docx.py, albeit none introduced changes to functionality.
  7. I've added tests/data/example_missing_column.csv to test our module catching a missing column in a csv file.

How to test Changes

Before

  1. Install nox on your machine globally, e.g. pip install --user --upgrade nox . If running nox results in a command not found, ensure the path to the install is available to your console. E.g. I had to add export PATH=/Users/<username>/.local/bin:$PATH to my ~/.zshrc file (I am using OhMyZ.
  2. Install pre-commit on your machine globally, e.g. pip install --user --upgrade pre-commit
  3. Run pre-commit install to set up pre-commit
  4. Run poetry install and poetry shell to update the dependencies (e.g. for pytest)

Functionality

Nothing has changed to the module's functionality, so it should still work with:

  • poetry run convert -t tests/data/example.docx -c tests/data/example.csv -n NAME
    (perhaps run poetry install and poetry shell beforehand)

Test suite

The following commands allow you to use the current setup:

  • poetry run pytest to only run tests using pytest
  • nox -r to run linting, safety and the pytests
  • nox -rs black to perform code formatting according to black (this will also be done for each commit with pre-commit
  • pre-commit run --all-files to run the pre-commit hooked actions without a commit (which will be executed for each git commit)

Next Steps

  • Some linting packages have not been added (yet), such as the suggested flake8-builtins, flake8-quotes, flake8-docstrings in Add linting and code formatting #10. These can be added still.
  • Type annotations as per issue Add type annotations and static type checking #11, e.g. following this guide.
  • Expand testing, such as the character tests as mentioned in Unit Testing Library  #23
  • Expand testing to validate document output, for example, whether .docx have been created (including edge cases).
  • All guides strongly suggest to set up the package using a /src/ folder - which we have discussed before. Potentially worth revisiting?

Review

Please review @jawrainey. In particular, whether adding the dev dependencies was appropriate (reflected in poetry.lock), and if you'd agree with this initials setup and approach.

@davidverweij davidverweij added the enhancement New feature or request label May 8, 2020
Copy link
Collaborator

@jawrainey jawrainey left a comment

Choose a reason for hiding this comment

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

I've ran nox (black, flake8, tests and coverage all independent and together) and pre-commit. All works well. Great work @davidverweij 👍

I've made some minor suggestions below that could improve the codebade, including the addition of the aaa flake8 plugin.

One important change not listed below that we always overlook: you forgot to update the README to explain how to run nox, linting, etc. Maybe we need to write a flake8 plugin for forgetting to update the README? 👍 (or add it to the PR template?)

csv2docx/csv2docx.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
@davidverweij
Copy link
Owner Author

Thanks @jawrainey! I'll go through the requested changes now and will update the pull request.

@jawrainey
Copy link
Collaborator

All guides strongly suggest to set up the package using a /src/ folder - which we have discussed before. Potentially worth revisiting?

@davidverweij -- could you also solve this? You'll need to:

  1. Create /src/ and move /csv2docx into it.
  2. Update code coverage config in pyproject to point to "src" instead of "csv2docx".

Remember to delete all .pyc or __pycache__ files/folders before running poetry as it might not run correctly otherwise

@davidverweij davidverweij mentioned this pull request May 9, 2020
@davidverweij
Copy link
Owner Author

1. Create `/src/` and move `/csv2docx` into it.

2. Update [code coverage config in pyproject](https://github.com/davidverweij/csv2docx/blob/unittests/pyproject.toml#L31-L32) to point to `"src"` instead of `"csv2docx"`.

Done, but no need for the pointer to change, as the init.py is still in the csv2docx folder.

@jawrainey
Copy link
Collaborator

jawrainey commented May 9, 2020

1. Create `/src/` and move `/csv2docx` into it.

2. Update [code coverage config in pyproject](https://github.com/davidverweij/csv2docx/blob/unittests/pyproject.toml#L31-L32) to point to `"src"` instead of `"csv2docx"`.

Done, but no need for the pointer to change, as the init.py is still in the csv2docx folder.

It is a bit confusing as no init is required in the src folder, e.g. see flask (web framework) as an example. So our convert in [tool.poetry.scripts] likely won't need to change. Why this works ... I am not certain, but can confirm it works as I tried it before out of curiosity.

@davidverweij
Copy link
Owner Author

davidverweij commented May 9, 2020

@jawrainey , I've hopefully addressed all requested changes - see my comments to your comments and in the change files. I've also moved the module to /src/, added a contribute to code and changed the pull request template both inspired on poetry's contribution.md.

Please review and let me know if you'd want any changes!

@jawrainey
Copy link
Collaborator

I've pulled a clean copy of the repo and ran poetry as usual. The new infrastructure in nox for testing and linting, and pre-commit all work well. Great job on setting this up @davidverweij and writing the CLI unit tests. I especially liked the use of usefixtures and the tweak to the PR template was genius 🤯

@jawrainey jawrainey closed this May 9, 2020
@jawrainey jawrainey reopened this May 9, 2020
@jawrainey jawrainey merged commit b01a378 into master May 9, 2020
@jawrainey jawrainey deleted the unittests branch May 9, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add linting and code formatting
2 participants