cs50 / checks

Checks for check50
17 stars 34 forks source link

Exceptions thrown in application.py do not get reported as 500 status codes when using Flask test app #34

Open cmlsharp opened 7 years ago

cmlsharp commented 7 years ago

Apparently when using the flask test app, exceptions thrown in the client do not result in 500 status codes as they do when running flask properly. Instead the exceptions propagate back up to check50 which results in the "check50 ran into an error running checks" message.

brianyu28 commented 7 years ago

Yeah, this is true. We can wrap our Flask methods in try/except blocks and raise errors that way, perhaps?

cmlsharp commented 7 years ago

It appears to be a consequence of this line:


        # initialize flask client
        app.testing = True

The purpose of this line is to let us use our own error handlers instead of the app's own. We could remove this line, and let the error handling that's already in the starter code for application.py return the apology. However, doing this actually revealed a bug in the way we set error handlers in the starter code:

The code that sets the error handlers is here:

def errorhandler(e):
    """Handle error"""
    return apology(e.name, e.code)

# listen for errors
for code in default_exceptions:
    app.errorhandler(code)(errorhandler)

The problem is, not every exception has the name/code fields. Raising a RuntimeError in index causes an AttributeError to be thrown in errorhandler when it tries to access the exception's name field which still ends up propogating up to check50.

Removing app.testing = True from check50 and changing errorhandler to look like:

def errorhandler(e):
    """Handle error"""
    try:
        return apology(e.name, e.code)
    except AttributeError:
        return apology(str(e), 500) # repr(e) is maybe more general, but it isn't as pretty

would solve it for Finance, though it's maybe a more general solution to keep app.testing and add our own handlers in check50 (though this complicates Finance a bit).

dmalan commented 7 years ago

Interesting, will dive in deeper to this this week.