add group/consumption_role invite/remove tests #1387
Conversation
| ) | ||
| finally: | ||
| if consumption_role: | ||
| assert_that(remove_consumption_role(client1, env_uri, consumption_role.consumptionRoleUri)).is_true() |
There was a problem hiding this comment.
Can we run an additional assert to ensure separate client (i.e. client2) cannot remove the consumption role that it does not own
| def test_invite_group_on_env_no_org(client1, session_env2, group3): | ||
| assert_that(invite_group_on_env).raises(GqlError).when_called_with( | ||
| client1, session_env2.environmentUri, group3, ['CREATE_DATASET'] | ||
| ).contains(group3, 'is not a member of the organization') |
There was a problem hiding this comment.
I believe group3 is invited to org2 which is where session_env2 is created in
Should this be group4 instead - in my testing this does not pass as is
There was a problem hiding this comment.
You are absolutely right, fixing it on next push.
|
Left some comments (mostly minor) on the files changed. Also when testing I think some of the organization tests may have broken from the changes made, mainly:
|
Thanks for reviewing Noah, indeed those tests were broken, my env was down and couldn't test them before pushing the PR. They are now fixed. |
Feature or Bugfix
Detail
Not fully tested since my environment is downRelates
Security
Please answer the questions below briefly where applicable, or write
N/A. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
evalor similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.