LGTM Devlog 25: Some cleanup
Tags: devjournal, python, testing
Published: 2021-01-24 11:45:39

I just realized my old branch wasn't merged in yet, and as I do that, my CI flags up coverage failure, since I'd set the test coverage minimum to 95%, and was getting lower than that.

![Coverage Failure](https://dev-to-uploads.s3.amazonaws.com/i/48e0c9lgpnd3dgnsczjq.png)

While you shouldn't live life by these numbers, these are indicators and should be looked into. If I deem there's a reason to allow it, I'd ignore it. The code for this post can be found at commit [48c6051](https://github.com/meseta/lgtm/tree/48c6051d732bb5d0839c547159ae35c5c336ba78)

First, I'm going to add a quick script into my `Pipfile` called `test_report` to help look at coverage results. Since pycov generates HTML reports, this line of code makes my life easier by spitting out the HTML report and starting a temporary webserver with python to let me view it. If I weren't using WSL, I could have this script pop open a web browser too

```
[scripts]
test = "pytest"
test_report = "bash -c 'pytest --cov-report=html && python -m http.server --directory htmlcov'"
deploy = "./deploy.sh"
```

Now I can have a browsable file tree containing coverage results![Test coverage report](https://dev-to-uploads.s3.amazonaws.com/i/cwnsf8zzzjdif6f9sxxr.png)

I need to set up my VSCode extensions to display coverage results at some point. For now this is an easy way to view my report

# firebase.py
Starting with Firebase.py, at 83% coverage, the reason is simple: I added in some conditional code to return the previously initialized firebase app/singleton, when in fact, putting this code in it's own module achieved the same effect, so it never ends up going into the branch. And since it's only 8 lines of code, drops the coverage by 12.5%.

![Missing coverage](https://dev-to-uploads.s3.amazonaws.com/i/7vycup61k2zw5mia1c6z.png)
 
I simply remove this whole `if` statement. Done

# main.py
Over at the main function, the first lack of coverage is an error handling when the hook repo didn't correctly match.

![Missing coverage](https://dev-to-uploads.s3.amazonaws.com/i/ri4qjzuu6fxli6q39dlw.png)

This indicates I'm missing a test where I present this function with a bad repo address. To adequately test this, I would need to generate some new hook payloads that are otherwise valid but have the wrong source repo. So I simply add an extra test case here.

![More missing coverage](https://dev-to-uploads.s3.amazonaws.com/i/wdvd6v5ep2q7zjgpbl9g.png)

In the next function, there's a few things going on, firstly a missing CORS header check. I don't have a test for CORS header handling. While I could add one, I've opted to instead eliminate this manual check and just use the `flask-cors` library that provides a decorator. Now instead of handling CORS manually, I let the decorator deal with it:

```py
from flask_cors import cross_origin

@cross_origin(
    origins=CORS_ORIGIN,
    headers=["Authorization", "Content-Type"],
    supports_credentials=True,
)
def github_auth_flow(request: Request):
    """ Validates a user from github and creates user """
    ...
```

The next item relates to handling when the provided user ID does not match the user ID that github returns as a token, this may happen in a spoofing attempt for example. For this I add another test that manipulates the user Id data provided to the function so that it is incorrect.

```py
def test_id_mismatch(auth_flow_client, test_user_token, user_data):
    """ Test a successful flow """

    user_data["id"] = "foobar"  # make id bad
    res = auth_flow_client.post(
        "/", headers={"Authorization": "Bearer " + test_user_token}, json=user_data
    )
    assert res.status_code == 403
```

Finally, the last remaining line not covered by testing is the event where a game already exists so that a user can be assigned. To achieve this, I need to go and create a new game for the user before anything else, so the test fixture now looks like this:

```py
def test_good_flow(auth_flow_client, test_user_token, user_data, test_auth_user):
    """ Test a successful flow """

    # make sure a game exists for user
    user = User.reference(Source.GITHUB, user_data["id"])
    game = Game.new(user, "fork_url")

    res = auth_flow_client.post(
        "/", headers={"Authorization": "Bearer " + test_user_token}, json=user_data
    )
    assert res.status_code == 200

    # check firestore
    doc = db.collection("users").document(test_auth_user.uid).get()
    assert doc.exists
    assert doc.get("id") == user_data["id"]

    # cleanup
    db.collection("game").document(game.key).delete()
    db.collection("system").document("stats").update({"games": firestore.Increment(-1)})

    # cleanup auto-created quest too
    QuestClass = Quest.get_first()
    quest = QuestClass()
    quest.game = game
    db.collection("quest").document(quest.key).delete()
```

There's a lot of cleanup in here. I can reduce a lot of this by re-using the fixtures already defined for creating users. I will do so in a future tidy-up, particularly if I can get the local Firestore emulator working so that I no longer have to test with the actual database (and therefore don't have to worry about cleaning up after test data).

# Automatically handle exceptions
Firebase's python functions framework has a way of registering exception handlers to automatically run to return values on exception, rather than having to explicitly return a json object. We can use this to keep the code tidy. For now, I will add a single exception handler for dealing with pydantic validation failures:

```py
@errorhandler(ValidationError)
def validation_error(err: ValidationError):
    """ Handler for pydantic validation errors (usuall http payload) """
    logger.error("Validation error", err=err)
    return jsonify(error="Validation error"), 400
```

Now, within each of the functions, I no longer have to explicitly handle the pydantic validationError, as the framework will run this function for me when that exception occurs.

# Dependency Injection
One thing I really miss about using FastAPI, and having to work with this kind of weird Flask-like implementation, is how FastAPI handle's dependency injection. So... let's roll our own. I use some of Python's introspection methods to detect whether there's a Pydantic model defined as any of the function arguments' type hints, and do a decode of the payload and inject it into the function, using a decorator:

```py
def inject_pydantic_parse(func):
    """ Wrap method with pydantic dependency injection """

    def wrapper(request: Request):
        kwargs = {}
        for arg_name, arg_type in get_type_hints(func).items():
            parse_raw = getattr(arg_type, "parse_raw", None)
            if callable(parse_raw):
                kwargs[arg_name] = parse_raw(request.data)
                logger.info(
                    "Decoded model and injected",
                    model=arg_type.__name__,
                    func=func.__name__,
                )

        return func(request, **kwargs)

    return wrapper


@inject_pydantic_parse
def github_webhook_listener(request: Request, hook_fork: GitHubHookFork):
    """ A listener for github webhooks """
    ...
```

In the above example, `github_webhook_listener()` has been wrapped by my new `inject_pydantic_parse()` function, which will detect that there is an extra `hook_fork: GitHubHookFork` argument, and since `GitHubHookFork` has a `parse_raw` callable, it will run `request.data` through it, and inject it as that argument.

Now, whenever one of my functions expects a json payload that I have a pydantic model for, I just specify it as an argument whose type hint is a pydantic object (or any class with a `parse_raw()' method, thanks to duck-typing). This keeps the code clean of duplicate pydantic model decoding and error handling.

___
And now, the CI reports 100% test coverage!

![100% test coverage](https://dev-to-uploads.s3.amazonaws.com/i/fprp93a8r1muzdnz0mz7.png)