add test for non-admin user #415
Conversation
kubevirt-bot
added
do-not-merge/work-in-progress
kubevirt-bot
removed
the
do-not-merge/work-in-progress
rhrazdil
left a comment
rhrazdil
left a comment
There was a problem hiding this comment.
Could you please add links to polarion test plans for the implemented test cases?
There was a problem hiding this comment.
I would prefer moving this logic to login method in utils, the login method should be improved to handle
- logging in kubeadmin (in both cases - with and without htpasswd identity provider enabled),
- logging in a different non-admin user (atm it's enough to only consider htpasswd)
- it must also take into account that when tests are executed with bridge UI, login screen is not displayed (so in that case it shouldn't be waiting for login form to appear)
There was a problem hiding this comment.
Put this logic into views/login.view.ts and simply the login function now.
frontend/integration-tests/tests/kubevirt/kubevirt.login.scenario.ts
Outdated
Show resolved
Hide resolved
frontend/integration-tests/tests/kubevirt/kubevirt.login.scenario.ts
Outdated
Show resolved
Hide resolved
frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
When running with bridge, there is no logout button, it should be enough to just do oc login with different user and reload page.
There was a problem hiding this comment.
oc login with different user and reload page does not work, it's not logging in a different user on UI.
frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts
Outdated
Show resolved
Hide resolved
frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts
Outdated
Show resolved
Hide resolved
frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
this could be simplified by a function; e.g. called like this:
withRole(role, name, namespace, () => {
await browser.get(`${appHost}/k8s/ns/${testName}/virtualmachines`);
await crudView.isLoaded();
...
})and similarly
withClusterRole(...There was a problem hiding this comment.
the inside of this method can be a parametrized function of 'Nonadmin user can use vm dialog in its own namespace'
|
Also, there is other functionality which should be tested for permissions / missing resources. |
There was a problem hiding this comment.
What is the benefit of this to be an inner function?
There was a problem hiding this comment.
I see some same usage about the inner function in modal-annotations.scenario.ts, so just try to use the similar thing here. Is it so bad?
There was a problem hiding this comment.
It is not that bad (:, just it would be easier to potentially reuse this function in the future.
There was a problem hiding this comment.
shouldn't we also check the yaml in VM Templates?
There was a problem hiding this comment.
The VM and templates are very similar. Can you please make it generic?
There was a problem hiding this comment.
it's checking whether user has permission to use wizard dialog and yaml dialog, VM has 'create from yaml' dialog but VM template doesn't have it.
There was a problem hiding this comment.
VM Template newly received this functionality in https://github.com/openshift/console
There was a problem hiding this comment.
this block would look better extracted into a function
There was a problem hiding this comment.
can you please move the perm arg to the end - it is easier to follow a pattern like this:
f(what, options)
There was a problem hiding this comment.
Will move the 'perm' arg to the end.
frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts
Outdated
Show resolved
Hide resolved
frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts
Outdated
Show resolved
Hide resolved
|
@gouyang , have you considered implementing new tests in openshift-console, please? There will be no other build of web-ui (except bug-fixes for 2.0.0). And as we have migrated almost all features from here to upstream already, we are moving integration tests is in progress as well these days. So targeting openshift-console directly sounds more effective to me atm. |
There was a problem hiding this comment.
Could we rather wait for some element to appear?
There was a problem hiding this comment.
I copied this from login.scenario.ts. It seems no necessary to sleep here as there are await in loginView.login.
There was a problem hiding this comment.
Wouldn't it be better if different permissions were tested in separate specs?
This verifyPermissions method seems too big and has no versatility. If there is a bug in some of the permissions, there is no way to specifically skip it.
This way we have passing something that should fail. It's better to skip a failing test with xit, as it's visible in test report.
There was a problem hiding this comment.
Separate specs means different describe, right?
I can change it like:
describe('Test nonadmin user UI behavior', () => {
describe('Test nonadmin user with roleBinding view UI behavior', () => {
describe('Test nonadmin user with roleBinding edit UI behavior', () => {
describe('Test nonadmin user with clusterRoleBinding view UI behavior', () => {
describe('Test nonadmin user with clusterRoleBinding edit UI behavior', () => {
And it does not need to change verifyPermissions.
There was a problem hiding this comment.
Okay, I will make changes to the PR in console.
There was a problem hiding this comment.
It would be better to just wait for the 'cannot list resource text' text. Check waitForStringInElement method in utils.ts and how it's used
There was a problem hiding this comment.
Use Wizard instance for this:
const wizard = new Wizard();
await wizard.openWizard();
await wizard.closeWizard();
I believe there are similar methods in the Yaml class
There was a problem hiding this comment.
Here we should rather wait for login screen with kubeadmin/httpass provider selection
There was a problem hiding this comment.
It can use that once 1721423 isn't blocking it.
There was a problem hiding this comment.
This I think should be broken down into multiple specs
I'm okay to move this to openshift-console, will move it over after address the review comments. |
Signed-off-by: Guohua Ouyang <[email protected]>
|
|
||
| export function createResource(resource) { | ||
| execSync(`echo '${JSON.stringify(resource)}' | kubectl create -f -`); | ||
| execSync(`echo '${JSON.stringify(resource)}' | kubectl apply -f -`); |
There was a problem hiding this comment.
Is this a good idea to mix declarative and imperative approach in creating resources? Wouldn't it be safer to have a new method, i.e applyResource?
There was a problem hiding this comment.
Well, the purpose of this change is avoiding to create a new method. Of course it can has another method or maybe just use execSync.
And why do we need createResource for one line code?
|
|
||
| export const login = async(providerName: string, username: string, password: string) => { | ||
| if (providerName) { | ||
| if (await $('a.idp').isPresent()) { |
There was a problem hiding this comment.
Note that this change probably should not be necessary in openshift/console
There was a problem hiding this comment.
Without this change, it can't run test with a cluster without idp.
It's not necessary if the test run with idp always, I know it's not perfect.
I have a PR in console about this: openshift#2016
|
Moved this to openshift#2265, will close this. |
5 participants
Implementation of Polarion test case:
https://polarion.engineering.redhat.com/polarion/#/project/CNV/workitem?id=CNV-1718
https://polarion.engineering.redhat.com/polarion/#/project/CNV/workitem?id=CNV-1720
Comments for review:
Signed-off-by: Guohua Ouyang [email protected]