-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add a few CLI tests #146
Add a few CLI tests #146
Conversation
@@ -10,6 +10,8 @@ matrix: | |||
- python: 3.6 | |||
env: TEST_TARGET=default | |||
- python: 2.7 | |||
env: TEST_TARGET=cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we make the CLI smarter we can test a full deployment in this matrix item. For now it only calls the commands and see if they don't return exit !=0
(fail).
- conda install --yes pytest | ||
- conda install --channel conda-forge --yes pycodestyle flake8-print flake8-quotes flake8-import-order flake8-builtins flake8-comprehensions flake8-mutable flake8-docstrings | ||
# Coding standards. | ||
- conda install --channel conda-forge --yes flake8-print flake8-quotes flake8-import-order flake8-builtins flake8-comprehensions flake8-mutable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed flake8-docstrings
to avoid unnecessary noise and place the mandatory test requirements in the requirement-dev.txt
def mkdtemp(): | ||
_directory = tempfile.mkdtemp() | ||
_stat = os.stat(_directory) | ||
yield _directory, _stat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lsetiawan this is a nice pytest
trick. This generator, as a pytest.fixture
, will create the test directory that will be removed only after the test is finished. It can be used with variables, files, etc, anything that you want to clean up after the use. Much less boilerplate than Python's unittest
module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. That's cool! Not very familiar with tests yet. But that makes sense.
|
||
def test_makedirs_do_not_overwrite(): | ||
_directory, _ = mkdtemp().next() | ||
with pytest.raises(OSError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test if the proper exception is raised when avoiding to overwrite.
_directory, _stat = mkdtemp().next() | ||
makedirs(_directory, overwrite='soft') | ||
stat = os.stat(_directory) | ||
assert _stat.st_ino == stat.st_ino |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test if the directory is untouched.
_directory, _stat = mkdtemp().next() | ||
makedirs(_directory, overwrite='hard') | ||
stat = os.stat(_directory) | ||
assert _stat.st_ino != stat.st_ino |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test if the directory was re-created from scratch.
if os.path.exists(directory) and overwrite == 'soft': | ||
return | ||
elif os.path.exists(directory) and overwrite == 'hard': | ||
sys.stdout.write('Overwriting directory {}\n'.format(directory)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler logic to avoid bugs and use stdout
instead of print
to be more CLI "friendly."
Windows is very conservative with its overwriting rules 😕
I need to read a little bit about it to fix this later. |
@ocefpaf, I don't remember the status of this PR, and it's not obvious from the sequence of comments. Looks like Don reviewed it a fair bit, then the last thing was the Appveyor/Windows issue. Should I merge? |
Sorry. I usually don't comment if the CIs are green after a fix to avoid noise. But I will do in the future.
Yep. The last commit fixed it. |
No worries. It fell under our radar screen -- weekend, Don on vacation, me busy. Merging now. |
Just should make future changes more robust and harder to break.