Contributing to diceware tool or 2018’s first Pull Request

You must have heard about diceware passwords before. If not, then the following image explains about it. It’s all about length of the password!

 

diceware passwords


I came across this tool called diceware which helps to generate these kind of passwords by using wordlists.  Kushal packaged this for Fedora, he wanted people to test his rpm build, so he posted link to the build on #dgplug. While testing, I just passed random string to the command:

(py36env)$ diceware nonexistingfile 
Traceback (most recent call last):
  File "/home/bhavin192/diceware/py36/bin/diceware", line 11, in <module>
    load_entry_point('diceware==0.9.4.dev0', 'console_scripts', 'diceware')()
  File "/home/bhavin192/diceware/diceware/__init__.py", line 211, in main
    print(get_passphrase(options))
  File "/home/bhavin192/diceware/diceware/__init__.py", line 184, in get_passphrase
    word_list = WordList(options.infile)
  File "/home/bhavin192/diceware/diceware/wordlist.py", line 120, in __init__
    self.fd = open(self.path, "r")
FileNotFoundError: [Errno 2] No such file or directory: 'nonexistingfile'
Exception ignored in: <bound method WordList.__del__ of <diceware.wordlist.WordList object at 0x7fe7b7b00710>>
Traceback (most recent call last):
  File "/home/bhavin192/diceware/diceware/wordlist.py", line 125, in __del__
    self.fd.close()
AttributeError: 'WordList' object has no attribute 'fd'

It shows whole stack trace instead of simple error message. There are two exceptions here, one is FileNotFoundError and the second one is AttributeError. Filed issue #43 on GitHub for this.


Sending Pull Request to solve the issue

As it fails to open the file it raises the first exception. Also, the variable fd never gets initialized. But the destructor tries to call it’s close() method. Pretty easy fix, right?

After thinking a bit I decided to catch the exception in the main() from __init__.py for first error and the second one got solved by adding one condition in __del__()

The code is compatible with Python 2 and 3. In Python 2, FileNotFoundError is not present. Instead, it raises IOError. Python 3 has OSError which has this exception, IOError is alias of OSError in Python 3. More about this

Created the PR #44 with following changes on 1st Jan,

This was showing the error and exiting with status code 1. As soon I pushed the code I realized that Travis CI failed. The reason was code coverage. So I started writing simple test case for this scenario, this was first time I wrote test using pytest

This runs the main function and catches the SystemExit, checks if the status code is 1, uses capsys fixture to read stdout and stderr and checks if the out has correct values.


Improvements in Pull Request

A week later, ulif (author) commented on the PR and he suggested some changes:

  1. It is an error message so it should go to stderr
  2. Possibility of using logger
  3. We should not catch all the IOErrors in main, it may catch other non related errors as well.

If logger (error/critical) is used, then the message goes to stderr. Solution for 3. is to check the errno attribute of the exception we are catching. List of errno symbols

So I modified the code with help of code from this comment

It checks if errno attribute of the exception matches to ENOENT and logs it as error with proper message. If it’s not ENOENT then it raises that exception as it is.

Now the tests, it was expected that err will have the message “The file nonexisting does not exist.”. It was having something else, which I discovered later. While searching for some way to assert over logs I found a in build fixture of pytest which is caplog.

This checks if number of log records is one. Then checks if the log level is ERROR and then checks the actual error message.

To cover the last raise statement ulif suggested me to generate some other IOError exception and check if it’s raised properly. To pass a directory, I used the tmpdir fixture. Again to maintain the compatibility between Python 2 and 3, it was necessary to catch it as IOError . pytest has something called ExceptionInfo, so to check the errno attribute we have to first use value which returns the actual exception. Read more about this


More changes

Everything seems to be perfect now, isn’t it? But wait, the fixture caplog was introduced in pytest-3.3. This change was introducing a dependency on pytest v3.3 which was causing Travis CI to fail for some Python versions. comment related to this

The err was not empty as I thought at the beginning. It was having really large output. ulif suggested to split the test in two tests as well as to use assert ... in. It worked as the large output was having the message we wanted to assert. This change eliminated dependency on pytest v3.3

And CI passed for all Python versions. PR took 38 days to get merged. Read the whole conversation, PR#44


Thanks to ulif for helping and replying politely even on silly mistakes.  ulif also added my name to the Credits section.

Things I learned

Trying to solve one issue can help you to learn a lot of new things.

Don’t forget to try diceware to generate passwords.  sudo dnf install diceware or sudo apt-get install diceware

 



, , , ,

Post navigation