Integration Tests: Adjusting Status Codes for Failing Tests - #23125
Conversation
|
Claude finished @NillasKA's task in 1m 36s —— View job PR Review
PR ReviewTarget: Corrects three expected HTTP status codes in the redirect URL deletion auth tests from No critical, important, or suggestion-level findings. The fix is correct and well-reasoned. Auth tests deliberately call the endpoint with a random ApprovedThis looks good to be merged as-is. Thanks for tracking down the root cause and for the clear explanation in the PR description! |
|
AndyButland
left a comment
There was a problem hiding this comment.
Thanks @NillasKA - looks good. I wondered whether we should do some data setup so we could continue to get an OK response, but then I considered that NotFound carries out the authorisation check just as well (and that's what these controller tests are about).



Description
This one kind of annoyed me when running tests on a different branch, and seeing these fail i had to fix them.
So this issue baffled me, because it seems the changes that caused this was introduced in my own PR #22985, that added
RedirectUrlOperationStatusto the deletion of redirects. This apparently messed up some of the tests. On my PR all tests passed, so that confused me a good amount, i still do not understand why or how, but regardless i see why they are failing now.So the issue here seems to stem from the fact that the
DeleteByKeyRedirectUrlManagementControllerTestswas used as part of the auth test suite to test permissions etc. TheMethodSelectorin these tests used to call a method that would just bevoidso not return anything and therefore status code OK was fine to test against, the caller had permission to call the method, no need to worry.Now with the
RedirectUrlOperationStatusthese tests returnNotFound, so the caller still has permission to call it, but the tests expect OK rather than NotFound. This PR adjusts the expected outcome of the method called.