OpenOversight icon indicating copy to clipboard operation
OpenOversight copied to clipboard

Surfacing form validation errors in tests

Open abandoned-prototype opened this issue 4 years ago • 3 comments

In a lot of our views that include submitting a form the flow looks something like this

@main.route('/unit/new', methods=['GET', 'POST'])
def add_unit():
    form = AddUnitForm()
    if form.validate_on_submit():
        # [...] create unit
        return redirect(url_for('main.get_started_labeling'))
    else:
        return render_template('add_unit.html', form=form)

The first time accessing the path we show the template with the form, then once it's submitted we do access the same path, but this time do something with the data and redirect to a different url. The problem is that with form.validate_on_submit() we don't separate the initial/GET case from the validation failed case. So in the case that the validation fails we return the original form, filled with the data and maybe some error message. This is a problem for our integration tests, because it is hard to say what actually went wrong since the error message (if it is shown at all) is hidden somewhere in the generated html code. For example the test_ac_can_edit_officer_in_their_dept test failed on the line officer = Officer.query.filter_by(last_name=last_name).one() but the reason was that the previous command

rv = client.post(
                url_for('main.add_officer'),
                data=data,
                follow_redirects=True
            )

seemed to have run successfully (status_code 200) when in fact it didn't do the expected work of editing the officer because the validation failed.

I am not sure how to best solve this, but maybe at least writing the validation errors to std-out or std-err might give some visibility while running tests.

abandoned-prototype avatar Aug 09 '20 00:08 abandoned-prototype

Tackled some of this in https://github.com/lucyparsons/OpenOversight/pull/783

e.g https://github.com/lucyparsons/OpenOversight/pull/783/files#diff-6e2fc07a8e3f096e2dbc42cbf2786db9R62

fritzdavenport avatar Aug 10 '20 00:08 fritzdavenport

It looks to me like a good portion of our tests check for errors by searching for error messages in the output, e.g. assert b'Email already registered' in rv.data, while assuming or asserting a 200 response code. Isn't that standard practice? I would expect form validation errors to still return a 200 response code and include form-wide or field-specific error messages inline.

dismantl avatar Aug 16 '20 17:08 dismantl

@dismantl in my experience, HTML requests typically return HTTP 400 or 422 with the validation errors, but a JSON API, for example, would always return HTTP 200 but include an errors key in the response object.

parhamr avatar Aug 23 '20 22:08 parhamr