We should never leak out exceptions in our controller tests

Dev
1

I just finished pushing a rather gnarly change per:

https://github.com/discourse/discourse/commit/49ed382c2a7ddd11756e552e094319861418cd7f

It basically replaces this pattern we used previously in tests:

 expect do
   get :index, format: :json
 end.to raise_error(Discourse::NotLoggedIn)

With this

get :index, format: :json
expect(response.status).to eq(403)

I made this change so our test environment is much more inline with our production environment. Previously we were leaking out a few exceptions and erroneously returning 500 status when we had a far more appropriate status to return.

From an API standpoint we never ever want exceptions to leak out, what we do want is to ensure correct error codes are returned to clients and that we don’t mutate data we should not for users that are not allowed access.

This change ensures that we stay “honest” and treat our tests just like we treat production.

So now our test environment has config.action_dispatch.show_exceptions = true just like we have in production.

And our test environment has config.consider_all_requests_local = false just like we have in production.

This will help catch future API inconsistencies going forward and various edge cases.

Note:

We still have this nasty in our specs

it 'requires a username_or_email parameter' do
   expect { put :create, format: :json }.to raise_error(ActionController::ParameterMissing)
end

This happens cause we never bother rescuing from ActionController::ParameterMissing and are erroneously returning 500s here (which are generated in the safeguard in unicorn), I went with a 400 for InvalidParams and should rescue this as well.

16 Likes