Executor ITF: transfer create_role_test #281
Conversation
| /** | ||
| * @given a user with all kCreateRole permission | ||
| * @when executes CreateRole command with empty permission set | ||
| * @then the command succeeds and the role is created |
There was a problem hiding this comment.
This case is stateless invalid. Looks like we need to connect stateless validation and execution in this framework, otherwise it is possible to write invalid cases.
There was a problem hiding this comment.
I don't think so. this is command and query executor backend test. it has its interface and it needs to be tested. if this test is out of scope of the command executor backend interface, I remove it.
There was a problem hiding this comment.
Alright, probably it makes sense in the context of configurable stateless validator.
| @@ -125,7 +126,11 @@ TEST_P(CreateAccountPermissionTest, CommandPermissionTest) { | |||
| ASSERT_NO_FATAL_FAILURE(getItf().createDomain(kSecondDomain)); | |||
| ASSERT_NO_FATAL_FAILURE(prepareState({})); | |||
|
|
|||
| if (checkResponse(createAccount(getActor()))) { | |||
| if (checkResponse(createAccount(getActor(), | |||
| kNewName, | |||
There was a problem hiding this comment.
Such approach is error-prone, because if default argument in createAccount is changes, it should be changed here as well. Maybe reorder arguments, or make another function? Same in other tests.
There was a problem hiding this comment.
added another function here. in other tests the other functions do not use the same default argument values, so I do not see any bad in this approach there.
| add_perm_case_if_provided(ActorRolePermissions::kEveryone, | ||
| permission_for_everyone); | ||
| add_perm_case_if_provided(ActorRolePermissions::kRoot, Role::kRoot); | ||
| add_case(ActorRolePermissions::kNone, |
There was a problem hiding this comment.
This code needs comments. It is not clear why there were more cases previously, and why it was possible to reduce them.
| /** | ||
| * @given a user with all kCreateRole permission | ||
| * @when executes CreateRole command with empty permission set | ||
| * @then the command succeeds and the role is created |
| /** | ||
| * @given a user with all kCreateRole permission | ||
| * @when executes CreateRole command with empty permission set | ||
| * @then the command succeeds and the role is created |
There was a problem hiding this comment.
Alright, probably it makes sense in the context of configurable stateless validator.
- accept additional perms for command issuer - readability of test case desctiptions - only create needed accounts - reduced amount of tests for grantable permission - added test case with disabled permission validation (for genesis block) Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>
Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>
MBoldyrev
force-pushed
the
feature/executor-itf-create_role_test
branch
from
d7b130c
to
a2dbd65
Compare
Signed-off-by: Mikhail Boldyrev miboldyrev@gmail.com
Description of the Change
transfer create_role_test from postgres_executor_test & BIG ITF acceptance to new shiny Executor ITF.
Benefits
Possible Drawbacks
Usage Examples or Tests [optional]
Alternate Designs [optional]