KEMBAR78
Isolate test files by bbc2 · Pull Request #160 · theskumar/python-dotenv · GitHub
Skip to content

Conversation

@bbc2
Copy link
Collaborator

@bbc2 bbc2 commented Dec 31, 2018

The test suite had the following issues:

  • A failing test would leave files in the tests directory, causing
    other tests to fail unexpectedly.
  • Even successful tests would require clean up code like sh.rm(...) to
    avoid leaving test files. This was easy to forget as shown by Deleted some temporary files that CLI tests were creating #153.
  • The same test file would be used for several tests, making them unsafe
    to run in parallel.

This commit fixes that by using the dotenv_file and tmp_path
fixtures whenever the filesystem is required. The advantage of those
fixtures is that they use unique test directories, which are eventually
cleaned up.

The tmp_path fixture is preferred to the tmpdir fixture because
tmpdir relies on py.path, a library in "maintenance mode".

The test suite had the following issues:

* A failing test would leave files in the `tests` directory, causing
  other tests to fail unexpectedly.
* Even successful tests would require clean up code like `sh.rm(...)` to
  avoid leaving test files.  This was easy to forget as shown by theskumar#153.
* The same test file would be used for several tests, making them unsafe
  to run in parallel.

This commit fixes that by using the `dotenv_file` and `tmp_path`
fixtures whenever the filesystem is required.  The advantage of those
fixtures is that they use unique test directories, which are eventually
cleaned up.

The `tmp_path` fixture is preferred to the `tmpdir` fixture because
`tmpdir` relies on `py.path`, a library in "maintenance mode".
@bbc2 bbc2 requested a review from theskumar December 31, 2018 17:57
assert os.environ[key_name] == 'WORKS'


@pytest.mark.xfail(sys.version_info < (3, 0), reason="test was incomplete")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@smsearcy This updated test shows the 2.7 dotenv detection bug on Travis (couldn't reproduce it on my machine though). Your fix in #152 makes it pass, which is a good sign.

Copy link
Contributor

@smsearcy smsearcy Feb 14, 2019

Choose a reason for hiding this comment

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

What's odd is that reviewing the coverage changes for #152 after merging #154 isn't showing it covering that code, and yet now the test passes. I'll dig into this a little more.

ETA: Based on the coverage docs it appears that there are some subprocess/threading calls that it cannot track, I suspect that is what is happening here because I confirmed via some print statements that we are definitely hitting the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new test unfortunately doesn't use the same Python VM, and so doesn't contribute to coverage. But this is also what makes this test isolated and realistic.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 88.502% when pulling b863de1 on bbc2:isolate-tests into cf9bd1f on theskumar:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 88.502% when pulling b863de1 on bbc2:isolate-tests into cf9bd1f on theskumar:master.

Copy link
Owner

@theskumar theskumar left a comment

Choose a reason for hiding this comment

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

Looks great! 💟

@theskumar theskumar merged commit 254d5bf into theskumar:master Jan 1, 2019
theskumar pushed a commit that referenced this pull request Jun 2, 2019
* master: (49 commits)
  Fix typo in README.md (#181)
  Refactor move run_command in cli
  Refractor: move 'to_env' to compat.py
  Refactor parser to fix inconsistencies (#180)
  chore(pypi): switch to markdown and twine for pypi upload + update supported version
  Bump version: 0.10.1 → 0.10.2
  readme: Add new release stub
  Fix unicode/str inconsistency in Python 2 (#177)
  Add argument to choose .env file encoding, defaults to None
  Fix links in readme
  Restrict typing dependency to Python < 3.5
  Add type hints and expose them to users
  Updated `.gitignore` with results from https://www.gitignore.io/api/python
  Added special case for `__file__` in Python 2.  Fixes #130 Updated tests to show that this works.
  Moved `dotenv` package into `src` directory so that tests run against installed version. Added tox to run test suite against multiple versions of Python (with coverage) and run flake8. Updated Travis CI to use tox as well. Updated documentation about running tests.
  Fix ResourceWarning: unclosed file in setup.py and test_cli.py
  Clarify the usuages of export
  Isolate test files (#160)
  Deleted some temporary files that CLI tests were creating.
  Add python 3.7 to testsuite + udpate pypi creds
  ...
@bbc2 bbc2 deleted the isolate-tests branch February 16, 2020 10:10
johnbergvall pushed a commit to johnbergvall/python-dotenv that referenced this pull request Aug 13, 2021
The test suite had the following issues:

* A failing test would leave files in the `tests` directory, causing
  other tests to fail unexpectedly.
* Even successful tests would require clean up code like `sh.rm(...)` to
  avoid leaving test files.  This was easy to forget as shown by theskumar#153.
* The same test file would be used for several tests, making them unsafe
  to run in parallel.

This commit fixes that by using the `dotenv_file` and `tmp_path`
fixtures whenever the filesystem is required.  The advantage of those
fixtures is that they use unique test directories, which are eventually
cleaned up.

The `tmp_path` fixture is preferred to the `tmpdir` fixture because
`tmpdir` relies on `py.path`, a library in "maintenance mode".
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.

4 participants