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

Handle FileNotFoundError with proper error message #44

Merged
merged 1 commit into from Feb 8, 2018

Conversation

Projects
None yet
2 participants
@bhavin192
Copy link
Contributor

bhavin192 commented Jan 1, 2018

As discussed in #43, now it shows proper error message if given wordlist file doesn't exist.
fixes #43

@bhavin192 bhavin192 force-pushed the bhavin192:infile-check branch 2 times, most recently from 42c8480 to 9d2998b Jan 1, 2018

@ulif

This comment has been minimized.

Copy link
Owner

ulif commented Jan 9, 2018

Dear @bhavin192 ,

thank you very much for the patch and sorry for the long delay!

While your patch does not raise any conflicts and looks fine in many respects, normally done wrong, I would still like to humbly ask for two or three further improvements of your pull-request, before I merge:

  1. It would be nice, if the error message could go to stderr instead of stdout.
  2. diceware provides a (yet nearly unused) logging mechanism that can be used for error messages:
     import logging
     logging.getLogger('ulif.diceware').error("My Error Message")

The default error level is probably too high currently("critical" IIRC), but I would fix that shortly.

  1. I am not sure whether catching all IOErrors in main is appropriate in that case. This would also catch other, unrelated IOErrors and give a wrong error message. So, why not check the condition ("file exists?") before and exit then (or raise an appropriate exception)?

If you could do something about at least the first point, that would be great.

Could you live with that?

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Jan 9, 2018

Thank you for suggesting improvements,

  1. I will do this one.
  2. Need to look into this.
  3. will write the solution again by keeping this in mind, raising custom exception can be a solution, I will look for options to catch only the FileNotFoundError exception
@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Jan 24, 2018

@ulif I'm thinking of creating a custom exception where should I put it? should I create a file named exceptions.py ?

@ulif

This comment has been minimized.

Copy link
Owner

ulif commented Jan 24, 2018

Hi @bhavin192
Do you think a custom exception is really neccessary? At least FileNotFound sounds like a proper, already existing exception type to me.

My impression was, that printing some error message to stderr would be more appropiate.

But if you really want to introduce a new exception type, then maybe logging.py or config.py might be places suitable for that kind of exception.

In the end, do not let me stop you. Just do, what you think is best.

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Jan 26, 2018

At least FileNotFound sounds like a proper, already existing exception type to me.

Python 2 does not have this exception, that's the problem.

@ulif

This comment has been minimized.

Copy link
Owner

ulif commented Jan 27, 2018

If it is only about catching "file-not-found" conditions in py2.x and py3.x, the follwing might spring to mind:

  1 import errno                                                                                                                                                                                                     
  2 import logging
  3 import sys
  4 
  5 try:
  6     open("not-existing-file", "r")
  7 except(OSError, IOError) as e:
  8     if getattr(e, 'errno', 0) == errno.ENOENT:
  9         logging.error(e)
 10         sys.exit(1)
 11     raise
 12 
 12 print("OK")

of course with the 'ulf.diceware' logger in place (snippet stolen from https://stackoverflow.com/a/39414133)

The solution above makes no assumptions about the error message, which makes it in turn possible to even leave out lines 8 and 11. I think this is easier and less error-prone than defining an own exception class, don't you think?

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Jan 27, 2018

Yes! right, I was thinking about similar solution instead of writing custom exception.

@bhavin192 bhavin192 force-pushed the bhavin192:infile-check branch from 9d2998b to eeeb9bc Jan 27, 2018

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Jan 27, 2018

I'm trying to figure out how to assert over the logged errors and also about covering the last raise statement.

@ulif
Copy link
Owner

ulif left a comment

Please forgive my nitpicking. If I can help with running the tests locally (with tox), please tell!

main()
assert exc_info.value.code == 1
out, err = capsys.readouterr()
assert out == "The file 'nonexisting' does not exist.\n"

This comment has been minimized.

@ulif

ulif Jan 28, 2018

Owner

You are looking in stdout for stderr output. This will hardly work ;)

This comment has been minimized.

@bhavin192

bhavin192 Jan 29, 2018

Author Contributor

The message was not in the stderr as well :( It gets captured in caplog, if I have understood it correctly. Reading this helped.

This comment has been minimized.

@ulif

ulif Jan 29, 2018

Owner

out, err are captured stdout, stderr. Hence the names.
assert err == "the file..." (instead of out==...) should therefore do the trick.

"The file '%s' does not exist." % infile_error.filename)
raise SystemExit(1)
else:
raise

This comment has been minimized.

@ulif

ulif Jan 28, 2018

Owner

The else branch is not covered in tests. This is revealed by coverage tests.

You can test all this on your commandline when running tox.

$ tox -e coverage

for coverage only, or just

$ tox

for running tests in all envs (including coverage and flake tests). If these tests pass, normally also travis will be happy.

This comment has been minimized.

@bhavin192

bhavin192 Jan 29, 2018

Author Contributor

Thank you, I'm using tox now. I'm still trying to figure out how can be the last raise statement covered.

This comment has been minimized.

@ulif

ulif Jan 29, 2018

Owner

In a test, try to provoke some IOError, OSError, which is not a file-not-found condition. Maybe you can request to use 'mylist' which you created as a directory before? Any kind of file-reading problem except file-not-found should do.

except (OSError, IOError) as infile_error:
if getattr(infile_error, 'errno', 0) == ENOENT:
logging.getLogger('ulif.diceware').error(
"The file '%s' does not exist." % infile_error.filename)

This comment has been minimized.

@ulif

ulif Jan 28, 2018

Owner

It is okay to invent a new error message text here, but not neccessary.

This comment has been minimized.

@bhavin192

bhavin192 Jan 29, 2018

Author Contributor

Any suggestions for different error message?

This comment has been minimized.

@ulif

ulif Jan 29, 2018

Owner

What about ...eror(infile_error)?

This comment has been minimized.

@bhavin192

bhavin192 Jan 30, 2018

Author Contributor

It would be like this then,
[Errno 2] No such file or directory: 'nonexisting'
what do you think?

This comment has been minimized.

@ulif

ulif Jan 30, 2018

Owner

Looks good to me 👍

@bhavin192 bhavin192 force-pushed the bhavin192:infile-check branch 2 times, most recently from 2a704d7 to 3b519bb Jan 29, 2018

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Feb 3, 2018

@ulif Hi
The logs are captured by caplog fixture, because of that out and err both are empty.
caplog was introduced in pytest-3.3
Other way I found is to use pytest-capturelog
I have Python 2.7, 3.4, 3.6 on my machine. tox reports no error as it installs pytest-3.3

@ulif

This comment has been minimized.

Copy link
Owner

ulif commented Feb 5, 2018

Hi @bhavin192,
caplog is supposed to capture output. If out and err are both empty: did you ensure, the loggers level is on logging.ERROR? If not, then your code base is outdated, I am afraid. I changed the default log level in one of the last commits.

I do not understand, what the pytest version has to do with it. If your local pytest version is outdated, just update with pip :) (I assume, you work in a virtualenv).

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Feb 5, 2018

did you ensure, the loggers level is on logging.ERROR? If not, then your code base is outdated, I am afraid. I changed the default log level in one of the last commits.

Yes, I have rebased my code, the level is on logging.ERROR
Also if I run like this in my virtualenv

$ diceware somename 2>./logfile 

The logfile has the expected error in it.

I do not understand, what the pytest version has to do with it. If your local pytest version is outdated, just update with pip :) (I assume, you work in a virtualenv).

I forgot to mention Travis CI, the pytest version on Travis CI is old for Python 3.4

@ulif

This comment has been minimized.

Copy link
Owner

ulif commented Feb 5, 2018

Ah, ok, now I see. Thanks for the reply! And I confused caplog with capsys.

In this case, as we actually want to check output on stderr, wouldn't it be sufficient or even more appropriate to use capsys?

(minimum versions of packages used in tests can be pinned in setup.py by the way).

@ulif

This comment has been minimized.

Copy link
Owner

ulif commented Feb 6, 2018

What about the following?

    def test_main_infile_nonexisting(self, argv_handler, capsys, tmpdir):
        # main() prints error if file does not exist
        sys.argv = ['diceware', 'nonexisting']
        with pytest.raises(SystemExit) as exc_info:                                                                                                                                                              
            main()
        out, err = capsys.readouterr()
        assert exc_info.value.code == 1
        assert "The file 'nonexisting' does not exist" in err

    def test_main_infile_not_a_file(self, argv_handler, tmpdir):
        # give directory as input
        tmpdir.mkdir("wordlist")
        tmpdir.chdir()
        sys.argv = ['diceware', 'wordlist']
        with pytest.raises(IOError) as infile_error:
            main()
        assert infile_error.value.errno == EISDIR

I hope, travis-ci would be happy with that (and we could avoid new test dependencies).

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Feb 7, 2018

Yeah! I will split those two, initially I was thinking about splitting those two tests.
So the err contains this message repeated several times,

View full output
--- Logging error ---
Traceback (most recent call last):
  File "/home/bhavin192/src/diceware/diceware/__init__.py", line 214, in main
    print(get_passphrase(options))
  File "/home/bhavin192/src/diceware/diceware/__init__.py", line 186, in get_passphrase
    word_list = WordList(options.infile)
  File "/home/bhavin192/src/diceware/diceware/wordlist.py", line 121, in __init__
    self.fd = open(self.path, "r")
FileNotFoundError: [Errno 2] No such file or directory: 'nonexisting'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib64/python3.6/logging/__init__.py", line 994, in emit
    stream.write(msg)
ValueError: I/O operation on closed file.
Call stack:
  File "/home/bhavin192/src/diceware/py36/bin/pytest", line 11, in <module>
    sys.exit(main())
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/config.py", line 58, in main
    return config.hook.pytest_cmdline_main(config=config)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 745, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 339, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 334, in <lambda>
    _MultiCall(methods, kwargs, hook.spec_opts).execute()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 614, in execute
    res = hook_impl.function(*args)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/main.py", line 139, in pytest_cmdline_main
    return wrap_session(config, _main)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/main.py", line 110, in wrap_session
    session.exitstatus = doit(config, session) or 0
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/main.py", line 146, in _main
    config.hook.pytest_runtestloop(session=session)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 745, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 339, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 334, in <lambda>
    _MultiCall(methods, kwargs, hook.spec_opts).execute()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 614, in execute
    res = hook_impl.function(*args)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/main.py", line 169, in pytest_runtestloop
    item.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 745, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 339, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 334, in <lambda>
    _MultiCall(methods, kwargs, hook.spec_opts).execute()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 613, in execute
    return _wrapped_call(hook_impl.function(*args), self.execute)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 248, in _wrapped_call
    call_outcome = _CallOutcome(func)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 265, in __init__
    self.result = func()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 613, in execute
    return _wrapped_call(hook_impl.function(*args), self.execute)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 248, in _wrapped_call
    call_outcome = _CallOutcome(func)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 265, in __init__
    self.result = func()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 614, in execute
    res = hook_impl.function(*args)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/runner.py", line 67, in pytest_runtest_protocol
    runtestprotocol(item, nextitem=nextitem)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/runner.py", line 81, in runtestprotocol
    reports.append(call_and_report(item, "call", log))
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/runner.py", line 157, in call_and_report
    call = call_runtest_hook(item, when, **kwds)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/runner.py", line 177, in call_runtest_hook
    return CallInfo(lambda: ihook(item=item, **kwds), when=when)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/runner.py", line 191, in __init__
    self.result = func()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/runner.py", line 177, in <lambda>
    return CallInfo(lambda: ihook(item=item, **kwds), when=when)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 745, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 339, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 334, in <lambda>
    _MultiCall(methods, kwargs, hook.spec_opts).execute()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 613, in execute
    return _wrapped_call(hook_impl.function(*args), self.execute)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 248, in _wrapped_call
    call_outcome = _CallOutcome(func)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 265, in __init__
    self.result = func()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 614, in execute
    res = hook_impl.function(*args)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/runner.py", line 111, in pytest_runtest_call
    item.runtest()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/python.py", line 1171, in runtest
    self.ihook.pytest_pyfunc_call(pyfuncitem=self)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 745, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 339, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 334, in <lambda>
    _MultiCall(methods, kwargs, hook.spec_opts).execute()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 613, in execute
    return _wrapped_call(hook_impl.function(*args), self.execute)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 248, in _wrapped_call
    call_outcome = _CallOutcome(func)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 265, in __init__
    self.result = func()
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 614, in execute
    res = hook_impl.function(*args)
  File "/home/bhavin192/src/diceware/py36/lib64/python3.6/site-packages/_pytest/python.py", line 143, in pytest_pyfunc_call
    testfunction(**testargs)
  File "/home/bhavin192/src/diceware/tests/test_diceware.py", line 326, in test_main_infile_nonexisting
    main()
  File "/home/bhavin192/src/diceware/diceware/__init__.py", line 218, in main
    "The file '%s' does not exist." % infile_error.filename)
Message: "The file 'nonexisting' does not exist."
Arguments: ()
The file 'nonexisting' does not exist.

and if I do as you said it works perfectly. Is that okay to do assert ... in over this large message?

Handle FileNotFoundError with proper error message
Using `IOError` for compatibility, from Python 3.3 `IOError` is alias of `OSError`
https://stackoverflow.com/a/28633573/6202405

Checking the errno of 'No such file or directory' so that only that exception will be catched.
https://docs.python.org/2/library/errno.html
https://stackoverflow.com/a/39414133

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>

@bhavin192 bhavin192 force-pushed the bhavin192:infile-check branch from 3b519bb to 5bc7030 Feb 7, 2018

@ulif

This comment has been minimized.

Copy link
Owner

ulif commented Feb 7, 2018

Nice! Travis-checks pass :-D

The mentioned assertion would actually be okay for me. But you are right: covering such a long output with this reduced assertion smells a bit fishy.

I tend to merge the PR anyway but would like to do some experiments before. This should be finished until tomorrow evening. I hope you can live with that last delay and thank you for your patience!

@ulif ulif merged commit 321aa0a into ulif:master Feb 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Feb 8, 2018

Finally! Thank you for guiding me @ulif :)

@ulif

This comment has been minimized.

Copy link
Owner

ulif commented Feb 8, 2018

You're most welcome, @bhavin192 !

Inspecting the lengthy py.test-problems closer, I discovered a different problem (log-handlers not cleaned up after runs), but this can (and has to) be handled as a different issue.

Thank you very much for your contribution and your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment