Refactor permissions integration tests #3124
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yndajas
force-pushed
the
refactor-permissions-integration-tests
branch
from
55407a6
to
c597121
Compare
yndajas
force-pushed
the
refactor-permissions-integration-tests
branch
6 times, most recently
from
2c41c28
to
2dbf43b
Compare
yndajas
force-pushed
the
fix-permissions-test-contexts
branch
from
134b5d7
to
d10a865
Compare
yndajas
force-pushed
the
refactor-permissions-integration-tests
branch
from
2dbf43b
to
3a62876
Compare
yndajas
force-pushed
the
refactor-permissions-integration-tests
branch
from
3a62876
to
6a435c8
Compare
These are covered in the `Account::ApplicationsControllerTest` spec. They don't add anything meaningful to that. We don't have equivalent tests for these in the `Users` namespace integration test spec
These were already partially covered by the controller specs, just not for both the apps you have access to and apps you don't contexts. It's overkill to cover these basic tests in both the integration and controller tests. Given they're just testing the rendered content of a page, and this being something we often test in controller specs in this repo, the controller specs feel like the most suitable home. This deduplication will allow us to simplify the integration tests a little There were no equivalent tests for this in the users integration test spec We might review and revise this approach later, but reducing the duplication/inconsistency for now should make it easier to work with and simpler to refactor later
While in practice we do need the app to have permissions in order to produce an update permissions link, we don't need to create them in the test setup here In practice, this method is only called in view templates that are accessible only to GOV.UK admins and publishing managers. The `update_permissions_link` method has some conditional logic that checks that an app has particular types of permissions dependent on which of those two role groups the user is in, ignoring normal users. We test this conditional logic in the tests for `update_permissions_link`, so I don't think it needs retesting in the tests for these methods (doing so would lead to some heavy context nesting) In the tests for these methods, the setup is already a little unrealistic: it uses a normal user, who - as mentioned above - wouldn't normally access view templates in which these methods are called. However, the conditional logic we want to test in this method relies solely on the output of policy methods, not users' roles, so I think this is okay. In using a normal user, we effectively bypass the role-based conditional logic in `update_permissions_link`, which means we also don't need any permissions on the app (sidenote: a non-delegatable permission would only work for GOV.UK admins, not publishing managers, due to the `update_permissions_link` conditional logic) So ultimately these tests assume that the app has permissions that the current user can manage, which I think is reasonable given the `update_permissions_link` tests cover the other cases
This brings these tests in line with the account ones and tests for other methods in this spec
We've got some heavy nesting going on in our applications controller tests at the moment, which makes them a bit unwieldy. I noticed that we're effectively duplicating nested contexts from the `ApplicationTableHelper` tests, which are in effect testing conditional logic from that helper's methods. I think we can assume that the unit tests for that helper will cover its own conditional logic, and we should then just test one happy helper path in the controller tests Note that the we do set up some of the policy requirements to reach the happy helper paths, but we don't give the app any permissions, which in practice would be required to reach these paths. However, like in some of the helper tests for its public methods, we use a normal user, which effectively bypasses some of the has manageable permissions checks in one of the helper's private methods. We're setting things up a little unrealistically in other areas, e.g. by opting for policy stubbing with a normal user rather than using a user with a more privileged role, so I think this is okay for now, but it's worth noting in case something changes that requires us to beef up the test setup in future
The integration tests and permissions controller tests were asserting on
slightly different things but with very similar tests. They were both
testing some part of the conditional logic, but it wasn't obvious to me
why each part would belong in either side
The permissions controller tests are updated to cover the following,
which was previously only tested in the integration tests:
- the second column ("Yes"/"No")
- behaviour for both GOV.UK admins and publishing managers
The ordering assertions are now a little more implicit, since they're
well tested in the `SupportedPermissionTest` and `ApplicationTest` specs
The integration tests were also testing that the view permissions link
appeared on the applications index. We previously tested this quite
thoroughly in both the applications controller and
`ApplicationTableHelper` test specs. A recent change left the latter
with responsibility for testing the complex conditional logic, and the
former with just checking they're rendered in a happy path. This
therefore remains well tested elsewhere
Note: we might revisit and refine the integration/controller divide
later, but for now I'm just opting to move towards consistency and
deduplication, which will hopefully make later more opinionated
refactors a little easier
yndajas
force-pushed
the
refactor-permissions-integration-tests
branch
5 times, most recently
from
e85de94
to
bf83770
Compare
This is a new consistent structure for the access and permissions tests. The actual test implementations will be ported over from the existing access and permissions tests gradually in later commits The specs are broken into three given the number of contexts that are covered. I'm not entirely convinced about how comprehensive to go with the condition testing, but this feels like a decent level of documentation and aligns reasonably well with the level in the associated Markdown documents (docs/access_and_permissions.md). This condition testing is well covered by unit tests, but it feels useful to cover it here too for those reasons Small note: I've ungrammatically gone for "a organisation_admin" in a few test descriptions to keep things consistent with descriptions generated in loops I'd like to keep things DRY within specs using shared methods for "should be able to" and "should not be able to" tests (i.e. two methods) and potentially between namespaces if there's any setup or set of steps that are the same regardless of whether your managing your own or others' access and permissions (shared methods can go in a support file) Given the tests for updating permissions for apps with more than eight permissions are essentially identical other than minor differences in setup, I've consolidated those into a single spec
These are a few general editing users helpers, which will be used across granting access, removing access, and editing permissions tests for navigating to and confirming that the user is on the account or users applications page, or that they can't access it
These will be used to keep similar account and users tests reasonably DRY
... to new structure, which provides coverage for different roles
... to new structure, which improves consistency between account and users namespaces and tests each role/condition more thoroughly
These will be used to keep similar account and users tests reasonably DRY
... to new structure, which provides coverage for different roles
... to new structure, which improves consistency between account and users namespaces and tests each role/condition more thoroughly
These will be used to keep similar account and users tests reasonably DRY
... to new structure, which improves consistency between account and users namespaces. The tests for when there are more than eight permissions will be moved over later
... to new structure, which improves consistency between account and users namespaces. The tests for when there are more than eight permissions will be moved over later
yndajas
force-pushed
the
refactor-permissions-integration-tests
branch
5 times, most recently
from
2ed3d2b
to
8b251e9
Compare
These will be used in the integration tests for updating permissions for apps with more than eight permissions
These are the tests for granting permissions for apps with nine or more permissions. Instead of making the account and users updating permissions specs really long and duplicative, I opted for having a single spec with a grantee who could either be the current user or another user. The tests here are even more alike between namespaces than tests for apps with eight or fewer permissions, which had some variation based on permissions/context Because the JavaScript tests need to set the JavaScript driver before running the setup (in particular logging in), the setup shared between JavaScript and non-JavaScript tests is encapsulated in a method rather than a `setup` block
yndajas
force-pushed
the
refactor-permissions-integration-tests
branch
from
8b251e9
to
fbce925
Compare
brucebolt
approved these changes
Sep 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello
A substantial refactor of the access and permissions integration tests, including revising the division between integration and non-integration tests, improving consistency between account and users tests (tests for editing your own and other users' permissions), breaking the tests down into much smaller specs for easier maintenance and navigation, and broadly writing much DRYer code. The new integration specs make heavy use of custom assertion methods mostly defined in helper modules, which hopefully make the tests more readable/consumable at a glance
This application is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.