[tests-only][full-ci] Get collaborator role using either button or span selector #7064
Conversation
saw-jan
changed the title
saw-jan
changed the title
| let currentSharePermissions = {} | ||
| try { | ||
| await collaboratorDialog.expandShareRoleDropdown(collaborator) | ||
| await this.selectRole('Custom permissions') | ||
| // read the permissions from the checkboxes | ||
| currentSharePermissions = await this.getSharePermissions() | ||
|
|
||
| // Hide role select dropdown | ||
| await this.moveToElement('@customPermissionsDrop', -9, 0) | ||
| await this.api.mouseButtonClick() | ||
| await this.waitForElementNotPresent('@customPermissionsDrop', 1000) | ||
| } catch (e) { | ||
| console.info('Collaborator Role is not editable!') | ||
| } |
There was a problem hiding this comment.
Since there is this #7063 issue on the web, I am less confident about this change.
And we don't have a test to check that after resharing with read-only permission, the collaborator role is listed as plain text. Tests won't catch the bug (#7063) in the future if this gets implemented.
Also, I am not sure whether the role should be listed as plain text or a drop-down after resharing with read-only permission.
There was a problem hiding this comment.
There was a problem hiding this comment.
It would be nice if we can work out why this different behavior happens!
@saw-jan do you see the problem manually? (Is it possible to observe both behaviors when slowly using the web UI manually?) (I want to rule out the chance that there is some transient display of UI elements which the automated test sometimes "sees" but for real users maybe the correct UI elements end up being displayed after a "flicker" of the incorrect UI elements)
There was a problem hiding this comment.
Yes, the issue can be reproduced manually. Steps to reproduce are listed here #7063
There was a problem hiding this comment.
I actually don't like to have this kind of workarounds in the test.
We had a similar discussion for the e2e tests in https://confluence.owncloud.com/pages/viewpage.action?pageId=28807613
what to do with bugs discovered by e2e tests?
- create an issue
- if possible run the journey without the step that fails
- if not then don't merge the test-PR that demonstrate the bug till the bug is fixed
We might want to make an exception here, because its not a failing test but a flaky one, but then we have to make super-sure that the workaround will be taken out when the bug is fixed
@kulmann what do you think?
saw-jan
requested review from
phil-davis,
kulmann,
pascalwengerter and
individual-it
| currentSharePermissions = await this.getSharePermissions() | ||
|
|
||
| // Hide role select dropdown | ||
| await this.moveToElement('@customPermissionsDrop', -9, 0) |
There was a problem hiding this comment.
can we get away without that position arguments? I know that has been there before (and even adjusted in between from -7 to -9).
We should get rid of it because
- it will always give us problems in the case of a slight change
- magic numbers are bad in the first place
but it could be done in a separate PR
| // Hide role select dropdown | ||
| await this.moveToElement('@customPermissionsDrop', -9, 0) | ||
| await this.api.mouseButtonClick() | ||
| await this.waitForElementNotPresent('@customPermissionsDrop', 1000) |
There was a problem hiding this comment.
we need to get rid of the fix timeout, this is a main thing why tests are becoming flacky AND slow. We might waste time here or maybe tomorrow 1000 will not be enough.
I know this was there before, so the refactoring could happen in an other PR
tests/acceptance/pageObjects/FilesPageElement/SharingDialog/collaboratorsDialog.js
Outdated
Show resolved
Hide resolved
| let currentSharePermissions = {} | ||
| try { | ||
| await collaboratorDialog.expandShareRoleDropdown(collaborator) | ||
| await this.selectRole('Custom permissions') | ||
| // read the permissions from the checkboxes | ||
| currentSharePermissions = await this.getSharePermissions() | ||
|
|
||
| // Hide role select dropdown | ||
| await this.moveToElement('@customPermissionsDrop', -9, 0) | ||
| await this.api.mouseButtonClick() | ||
| await this.waitForElementNotPresent('@customPermissionsDrop', 1000) | ||
| } catch (e) { | ||
| console.info('Collaborator Role is not editable!') | ||
| } |
There was a problem hiding this comment.
I actually don't like to have this kind of workarounds in the test.
We had a similar discussion for the e2e tests in https://confluence.owncloud.com/pages/viewpage.action?pageId=28807613
what to do with bugs discovered by e2e tests?
- create an issue
- if possible run the journey without the step that fails
- if not then don't merge the test-PR that demonstrate the bug till the bug is fixed
We might want to make an exception here, because its not a failing test but a flaky one, but then we have to make super-sure that the workaround will be taken out when the bug is fixed
@kulmann what do you think?
saw-jan
force-pushed
the
collaborator-role-selector
branch
from
f1cfd10 to
f28e192
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@saw-jan, thanks for the PR, the linked bug is already in our Beta board, lets see when this will be fixed, from my side i would love to see it fixed inside web which then would make this pr obsolete. cc.: @phil-davis |
|
I moved this to "blocked" and removed review requests. We will see this week if the underlying problem with the web code can be fixed. |
|
The main issue has been fixed in the source code: #7063 |
Description
The collaborator role can either be a dropdown or a plain text according to the share permissions. In this PR, I have refactored the
getCollaboratorsListfunction to get the collaborator role either from a span or a button element.Also, added the code that gets the permissions from a dropdown inside a try-catch block and returned empty permission if there is no dropdown elements.
Related Issue
Changes made in #7015
Closes #7047
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: