An example development cycle for fixing a bug

As I’m using my own code for various tasks I notice that

./scripts/sim_catalog --ref_ra=00:42:44.3 --ref_dec=-00:30:19 --radius 1 --n 1000

produces some unexpected output. A plot of the sky locations is shown below. Note that the points have been generated around a central declination of 00:30:19 instead of -00:30:19. It seems that there is an issue with a negative reference declination.

Reporting the issue

As a user, once I have identified an issue I should go to github and make a bug report on the issue tracker. In the example project I’m the only on the developer team so I’ll assign myself to the issue. I’ll also label it as being a bug.

Confirming the issue

Now that the issue has been submitted I’ll tackle this problem from the developer point of view.

The first thing to do is read and understand the issue. In this case I’ll just run the script exactly as shown in the issue tracker, and see that I get the same problem.

Create a feature branch for this issue

At this point I should create a new branch in my repository with some relevant name. Since the names of issues are not guaranteed to be unique I will instead use the issue number (#1 in this case) as part of the branch name. For a project with multiple developers it is also a good idea to identify who is the ‘owner’ of each branch. An example branch choice would be:

git branch Paulhancock/Issue#1

After some trial and error I find that the cause of the bug is in the following code:

def generate_positions(ref_ra='00:42:44.3',
                       ref_dec='41:16:09',
                       radius=1.,
                       nsources=1000):
...
    # convert DMS -> degrees
    d, m, s = ref_dec.split(':')
    dec = int(d)+int(m)/60+float(s)/3600
...
    return ras, decs

The first thing that I do is to make a new test that will expose this bug.

Writing a test

We will now write our first formal test for our code. We’ll use a format that will make future automated testing easier. All the tests that we wish to run are python scripts, and we’ll place them in the tests/ directory. Each script will test a different sub-module, and for the sim sub-module of our skysim module, we’ll collect all the tests into tests/test_sim.py.

The first thing that we need to do is import the module/code that will be tested. In this case that will be the generate_positions function within the skysim.sim module.

#! /usr/bin/env python3
"""
Tests for the skysim.sim module
"""
import numpy as np
from skysim.sim import generate_positions

Each test that we write will be contained within a separate function whose name begins with test_, and which returns None when the test passes, and raises an AssertionError if the test fails. While we could home-brew our own set of standards for what pass/fail looks like, we will instead use standards set out by one of the common python testing frameworks called pytest.

We craft a piece of code that will detect the mistake in our original function. In this case the mistake is that the negative sign at the start of the declination is being ignored so we get the wrong positions. To test for this we’ll run generate_positions with a declination that is negative, and small radius, so that the expected output should consist entirely of negative declinations if the function works properly, and probably all positive declinations if it’s broken.

Our test function looks like this:

def test_negative_dec():
    """
    Test for the negative dec bug noted in issue #1
    """
    _, decs = generate_positions(ref_ra='00:00:00',
                                 ref_dec='-00:30:19',
                                 radius=0.1, nsources=10)
    if not np.all(decs < 0):
        raise AssertionError("Declinations should be <0, but are >0")
    return

In order to run the tests we can add the following snippet to the end of our script. The snippet essentially looks at all the global variables (including function names), selects those that start with test_, assumes that they are a function and calls that function. When the function is called there is a try/except for an AssertionError which reports failure if it’s caught, or reports success if no error was raised.

if __name__ == "__main__":
    # introspect and run all the functions starting with 'test'
    for f in dir():
        if f.startswith('test'):
            try:
                globals()[f]()
            except AssertionError as e:
                print("{0} FAILED with error: {1}".format(f, e))
            else:
                print("{0} PASSED".format(f))

When we run our test code we get the following result:

$ python tests/test_sim.py 
test_negative_dec FAILED with error Declinations should be <0, but are >0

This failure is not a bad thing, it means that we have successfully written a test function that will identify the bug. Now we can begin the process of fixing the bug.

Fixing the bug

Finally, once I have the test code in place, it’s time to fix the bug. I make some modifications to account for the leading minus sign on the declination as follows:

    # convert DMS -> degrees
    d, m, s = ref_dec.split(':')
    sign = 1
    if d[0] == '-':
        sign = -1
    dec = sign*(abs(int(d))+int(m)/60+float(s)/3600)

And I then re-run the code to make sure that the bug has been resolved, and then run my tests:

$ python tests/test_sim.py 
test_negative_dec PASSED

As I develop more and more tests the list of functions run will grow. Once the new bug has been solved I will re-run all my tests to ensure that fixing this bug has not caused a new bug some other place.

Checking in my work

I now check in my new test code, and updated version of sim.py:

git add tests/test_sim.py
git commit -m 'expose bug from issue#1'
git commit -m 'resolve #1' skysim/sim.py

Note that I have used #1 to refer to the issue from within my commit message. When viewed on Github these commit messages will automatically generate a link to the issue, and when viewing the issue I should be able to see the reverse link.

I now push the bug fix (and my new branch) to the Github repo.

git push --set-upstream origin Paulhancock/Issue#1

If we look on the original issue page, we can see the link to the commit.

Creating a pull request

When we navigate to the landing page for our repository we will see a new yellow banner appear as below:

We can click the green “Compare & pull request” button to start a new pull request. Alternatively we can go to the “Pull requests” tab. Either way we enter a title and description for the pull request.

Note that the assign/label/project/milestone options that we see on the pull request form are mostly the same as on the Issues form. This is because pull requests are just special types of issues. They share a numbering scheme. This is the first pull request for this repository but it will be labelled #2 because there is an existing issue #1. One difference between a pull request and an issue is that a pull request can have a reviewer assigned to it. Here I have selected myself as the assignee (the person looking after the pull request), and SkyWa7ch3r as the reviewer (the person who will review my code and sign off when they are happy).

Github does some work in the background to let me know that there will be no conflicts between this branch and the main branch, so that it is ‘safe’ to do the merge. Currently there is no indication that the code works or passes our tests. For now we let the reviwer do this work. The reviewer would pull the Paulhancock/Issue#1 branch, run the tests and see that they pass, then come back to github and make a note of it in the discussion. (In a later lesson we’ll see how we can make Github do most of this work for us using Github actions.)

Once our reviewer(s) are happy with the changes we can merge our branch back into main by pressing the green button. This will create a new commit on the main branch in order to do the merge, so we’ll be asked for a title/description for the commit. It is pre-filled for us. Once the merge is complete Github will let us know that all is good, and suggest that we delete the branch. Since the feature is merged we no longer need this branch and will delete it.

Closing the issue

If we navigate back to the issues tab, we’ll see that the issue related to the pull request has also been closed for us. This is because we wrote resolve #1 as a comment for our commit. When that commit is merged into the main branch git will automatically resolve (close) the linked issue. If we didn’t use this smart linking capability we can still go back to the open issue and close it. Either way it would be good to leave a note about the issue being fixed.

Summary

The development cycle for fixing a bug is as follows:

• identify bug
• report bug on the Gihub issue tracker
• confirm that the bug exists
• create a feature branch
• write a test to expose the bug/error
• fix the bug
• run all tests
• commit changes
• create a pull request
• merge the branch into main and delete the feature branch

In this example we had one person doing the reporting/fixing. Usually you’ll have an end user doing the finding/report part, and then one or more developers doing the remainder.