chore : Comprehensive e2e testing for public sharing #50260
Conversation
nfebe
force-pushed
the
enhancements/files-sharing-tests
branch
2 times, most recently
from
d7f2b84 to
153862d
Compare
This update enhances the testing framework for public sharing by improving the setup and management of share contexts. It refactors the code to make the `createShare` function reusable externally, ensuring better testability and flexibility. Additionally, the update optimizes share permission adjustments and improves state management, making public share setups more efficient and maintainable. Signed-off-by: nfebe <[email protected]>
nfebe
force-pushed
the
enhancements/files-sharing-tests
branch
from
153862d to
088f24e
Compare
This commit introduces end-to-end tests for various public link sharing scenarios in the `files_sharing` app. It validates the behavior under multiple configurations: - Password and expiration date enforced - Password enforced with a default expiration date - Expiration date enforced with optional password - Default password and expiration date without enforcement - Password enforced, expiration date set but not enforced - Both password and expiration date not enforced, but defaults set - Password not enforced, expiration date enforced - Password not enforced, default expiration date set - Password and expiration date not enforced with no defaults The tests ensure proper validation and functionality of the configurations, leveraging the `setupData` and `createShare` utilities. Signed-off-by: nfebe <[email protected]>
Signed-off-by: nfebe <[email protected]>
nfebe
force-pushed
the
enhancements/files-sharing-tests
branch
from
088f24e to
2133889
Compare
nfebe
changed the title
nfebe
changed the title
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| export type ShareOptions = { |
There was a problem hiding this comment.
Not sure an additional file is needed as this directly belongs to the helper scripts. IMHO it could go directly into that file
There was a problem hiding this comment.
Yes I contemplated that but ended up with this because I thought, next we need to write tests for internal sharing (SharingDetailsTab) and these type would be handy there.
Sure we could import from public sharing but I thought this is cleaner. What do you think?
| cy.contains('Enter a password') | ||
| .should('exist') | ||
| .and('not.be.disabled') |
There was a problem hiding this comment.
Can be extracted from the if/else if it is the same as in the else statement.
| cy.contains('Enable link expiration (enforced)').should('exist') | ||
| cy.contains('Enter expiration date') | ||
| .should('exist') | ||
| .and('not.be.disabled') |
There was a problem hiding this comment.
Can be extracted from the if/else if it is the same as in the else statement.
| cy.findByRole('list', { name: 'Link shares' }) | ||
| .findAllByRole('listitem') | ||
| .first() | ||
| .findByRole('button', { name: /Actions/i }) | ||
| .click() | ||
| cy.findByRole('menuitem', { name: /Customize link/i }) | ||
| .should('be.visible') |
There was a problem hiding this comment.
Should be kept to ensure that the button is not hidden by something. If it is, then this is a problem.
There was a problem hiding this comment.
But isn't it the default behavior of .click()? 🤔
|
|
||
| return cy.task('getVariable', { key: 'public-share-data' }) | ||
| .then((data) => { |
There was a problem hiding this comment.
Usually we keep the .then(...) on a new line.
| const { dataSnapshot, shareUrl } = data as any || {} | ||
| if (dataSnapshot) { | ||
| cy.restoreState(dataSnapshot) | ||
| defaultShareContext.url = shareUrl |
There was a problem hiding this comment.
If you set one of its property, it is not a default anymore.
There was a problem hiding this comment.
I don't get the purpose of defaultShareContext. It is a global variable that is used only in this file, but is also passed as an argument in the functions in this file. Feels confusing. And it is mainly used to provide the User object and to receive the url. Can't we use an argument and the return value instead?
Can't we do without?
| cy.mkdir(user, `/${shareName}/subfolder`) | ||
| cy.uploadContent(user, new Blob(['<content>foo</content>']), 'text/plain', `/${shareName}/foo.txt`) | ||
| cy.uploadContent(user, new Blob(['<content>bar</content>']), 'text/plain', `/${shareName}/subfolder/bar.txt`) | ||
| export function setupData(context: ShareContext, shareName: string): void { |
There was a problem hiding this comment.
No need to provide more.
| export function setupData(context: ShareContext, shareName: string): void { | |
| export function setupData(user: User, shareName: string): void { |
| export function createShare(context: ShareContext, shareName: string, options: ShareOptions | null = null) { | ||
| cy.login(context.user) | ||
| cy.visit('/apps/files') // Open the files app | ||
| openSharingPanel(shareName) // Open the sharing sidebar |
There was a problem hiding this comment.
| openSharingPanel(shareName) // Open the sharing sidebar | |
| openSharingPanel(nodeName) |
| afterEach(() => { | ||
| cy.runOccCommand('config:app:delete core shareapi_enable_link_password_by_default') | ||
| cy.runOccCommand('config:app:delete core shareapi_default_expire_date') | ||
| cy.runOccCommand('config:app:delete core shareapi_enforce_expire_date') | ||
| cy.runOccCommand('config:app:delete core shareapi_expire_after_n_days') | ||
| }) |
There was a problem hiding this comment.
Shouldn't we also reset shareapi_enforce_links_password?
| cy.runOccCommand('config:app:set --value yes core shareapi_enable_link_password_by_default') | ||
| cy.runOccCommand('config:app:set --value yes core shareapi_enforce_links_password') | ||
| cy.runOccCommand('config:app:set --value yes core shareapi_enforce_expire_date') | ||
| cy.runOccCommand('config:app:set --value yes core shareapi_default_expire_date') | ||
| cy.runOccCommand('config:app:set --value 2 core shareapi_expire_after_n_days') | ||
|
|
||
| const shareName = 'passwordAndExpireEnforced' | ||
| setupData(shareContext, shareName) | ||
| createShare(shareContext, shareName, { | ||
| alwaysAskForPassword: true, | ||
| enforcePassword: true, | ||
| enforceExpirationDate: true, | ||
| defaultExpirationDateSet: true, | ||
| }).then((shareUrl) => { | ||
| shareContext.url = shareUrl |
There was a problem hiding this comment.
Are the ShareOptions a reflection of the options set by occ? If so, we should make this more obvious, by either constructing the ShareOptions from the settings value, or vice versa.
This PR enhances the testing framework for public sharing by introducing modular improvements and extending test coverage with "before create public share" checks.
Adds the following tests:
Screencast for e2e plays
public-sharing-e2e-tests.mp4