Review Board 7.1 alpha 0 (dev)

Hi, my name is Calvin Shin, and I am a former FB Open Academy student who was 

assigned with fixing a bug, and I would like to explain how I fixed this bug. 

Let's get to it!

The initial issue at hand was that as an admin of Review Board, you could not 

add a private Github or Kiln repository. It displayed a message that read 

" 'URLError' object has no attribute 'read()'." Once I tackled this, I 

changed the error message so that it could properly convey why the error was occurring.

Now, for a step-by-step process on how I tackled this HostingService bug-fix. 

First, the error message that popped up when I reproduced this bug was a good 

indicator of where I should look in the code to start. Keep in mind the scale 

of the Review Board codebase in comparison to that of your class assignments 

and projects. So the error message really saved me a lot of time. Debugging 

can really get you lost for hours with a codebase like Review Board. 

Anyways, proceeding with this "hint," I did a quick find in Pycharm, and 

along with the help of a mentor at my school's Code Sprint, I found the 

section of the code where it could have been producing that message to 

appear. This lead me to a part of the code in github.py, into a function 

called 'authorize'. I looked around in this function and saw the line 

"data = e.read()". At this point, I saw that the line was in a clause to 

handle a HTTPError and a URLError and thought that one possibility (deducing 

from the error message) could be that e was a URLError and that the line 

"data = e.read()" was causing this message because a URLError object doesn't 

have the ability to call the read() function. Confirming this with a mentor, 

I fixed this by making a simple change. I put the exact same line inside a 

try block that was previously right underneath it. My reasoning for this was 

that it was failing because data contains an invalid statement (URLError 

doesn't have attribute read) and so putting it in the try block would attempt 

to execute that line, but if it didn't work, it would just go ahead to the 

except block, which was right after the try block. My mentor said to make 

the same changes where there were other areas of the code that had similar 

lines, and so I made the same changes in another function in github.py called 

'_check_api_error', and a function in kiln.py called 

'_check_api_error_from_exception'. I tried reproducing the bug with the same 

steps to see if I had fixed the bug, but this time I encountered another 

error. This one gave another error message reading 'URLError object has no 

attribute code'. This time, I traced the code back to '_check_api_error', 

because that was the only place out of the fixes in github.py that I made 

that contained any text of "code." I saw that the variable e, which was 

assigned URLError, was calling something that it couldn't; this time, 

something called code. Only HTTPError, a subclass of URLError, has attribute 

"code," so I fixed this problem by handling these two errors separately 

(because both HTTPError and URLError could be assigned as e). If you take a 

look at '_check_api_error' now, you can see that HTTPError and URLError are 

handled separately. This is the last set of if-else statements in the 

function. I changed it so that the HostingServiceError, which previously had 

the variable http_code=e.code as its second parameter, does not have a second 

parameter and instead passes in the default parameter set for http_code, 

which is assigned None. This finally allowed Review Board to display the 

correct reason for the inability to add a Github repository, but this message 

showed "<urlopen error [Errno 61] Connection refused>". Because this error 

message has no significance whatsoever to a user of ReviewBoard, I changed 

this to read "Could not connect to the server "<domain name>". If the server 

is behind a proxy, make sure the proxy settings are correct." This message 

essentially says tells the user that the reason that he or she cannot add a 

repository is because Python isn't properly configured to a proxy server.

Cool, so it seems like the bug is fixed, but this is only the halfway mark in 

a sense. Now that we have what we think is working, we need to verify with 

unit tests. Unit tests ensure that whatever behavior we are expecting works 

properly and accordingly and does not have any changes. This was the part I 

had the most complications with, mainly because I was required to trace a lot 

of code and "deep-dive" into code I wasn't working with or familiar with. On 

top of all this, I only had minimal experience writing my own unit tests for 

working code. 

Let's take a quick detour to give a sense of how to trace code. In Pycharm, 

you can use a shortcut by pressing on the cmd (Macs only) key and clicking on 

a variable or a function or a class. This will directly lead you to that, in 

whichever file it is in. For instance, let's take a look at a function called 

'get_file_exists', in kiln.py, which is an example of an API call (explained 

in the next paragraph). Starting with this function, you can see that it 

returns True, if it successfully calls another function called 'get_file'. 

Using the keyboard shortcut will directly go to the function in whichever 

file it is contained in. This shortcut leads you to the 'get_file' function, 

and observing it, you can see that it calls another function called 

'get_raw_file', which in turn calls 'api_get' on a url. Finally, this calls 

'http_get' on that url, which performs a GET request on that url. This is 

assigned to a variable called data and returned. 

Going back to my own unit test, I started off with writing an individual unit 

test for each individual fix. So for instance, I made two fixes in github.py 

and one fix in kiln.py, even though they were contributing to just one bug. 

I wrote 3 individual unit tests, two to test using a Github repo and another 

one to test using a Kiln repo. I will explain my writing of the unit tests 

using the github unit test that tests '_check_api_error'. I first started 

off with writing the unit test function signature, which is simply 

'def test_check_url_error (self)'. All unit tests in Python begin with test 

and do not have other parameters. Secondly, I used the concept of a "spy." 

A spy essentially intercepts and records calls to functions (more about 

function spies here: https://github.com/beanbaginc/kgb). In this case, I 

"spy on" the http_get function and faked the original function so that it 

will raise a URLError, for the purposes of testing our code. After this, I 

retrieve the proper hosting service and finally call assertRaisesMessage, 

passing in the error message that I fixed, so that the test can extract a 

known result. In the call to assertRaisesMessage, I make an API call by 

calling '_test_check_repository' (which calls '_check_api_error'), so that 

the unit test can actually test something that the HostingService would do. 

In this case, it is to check a repository; this function makes a proper API 

call because it reaches 'api_get', which calls 'http_get', the function that 

is spied on to raise a URLError. 

Ultimately, this bug that I thought I had fixed at the code sprint, or at 

least by the end of my first week, resulted in me working on this as 

essentially another project, along with the project I had chosen to work on 

throughout the term. I honestly was quite annoyed when everybody else 

finished their bug fixes their first week and spent the majority of their 

time on their individual projects. However, from this one bug, I learned how 

to trace and follow code, from one function to another, in a huge project 

with a lot of files and lines of code, an invaluable skill for any developer. 

I learned how to write unit tests, also an essential skill in industry. 

Finally, I gained insight into debugging, which is, in and of itself, another 

aspect of software engineering. Thank you for reading this, and I hope you 

are now equipped to debug your bugs!