fix(tests): Move leftover acceptance tests for users from drone to Cypress

[susnux]

Summary

Move the tiny rest of acceptance tests for user setting to Cypress and add missing test cases:

• Change quota
• Set subadmin
• Add manager

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[susnux]

BTW using random users should make it also less flaky, as now every test has a defined state.

[skjnldsv]

BTW using random users should make it also less flaky, as now every test has a defined state.

If random users makes it flaky, the tests or our codeis probably the issue then.
I would prefer not hardcoding users, they do create a new universal "fresh Nextcloud" base and we should rely on it :)

[susnux]

I would prefer not hardcoding users

Exactly, previously hardcoded "jane doe" was used, but that might lead to inconsistent state.
Now random users are created, so at least the user does not exists and we start with a fresh profile.

It would need some more work (I do not have the time currently) to rewrite the tests, as the best way (and the best practice from Cypress) would be to ensure we have a clean state before the tests and not clean up after the tests.

So e.g. removing the datadir and resetting the database before each test.

Thinking about this hardcoding users might also be good if we always use the same and have something like:

before each: delete user "test" if exists. Recreate a fresh user "test".

But especially as our Cypress tests are also changing admin settings (we have a reset but only for theming), the golden standard would be to reset all settings by running every testsuite with the initial database.
So not sure if possible but creating a snapshot of the datadir right before cypress starts (as that includes the sqlite database) and then having a command that resets the datadir to the snapshot in the before hook.
So something like this (did not test probably does not work):

// in e2e support file
before(() => {
	cy.exec('docker exec --user www-data nextcloud-cypress-tests-server rm -rf apps-cypress')
	cy.exec('docker exec --user www-data nextcloud-cypress-tests-server mkdir apps-cypress')

	cy.exec('docker exec --user www-data nextcloud-cypress-tests-server bash -c \'echo -e "<?php\\n\\$CONFIG = [\\n\\t\\"apps_paths\\" => [\\n\\t\\t[\\"path\\" => \\"/var/www/html/apps\\", \\"url\\" => \\"/apps\\", \\"writeable\\"=>false],\\n\\t\\t[\\"path\\" => \\"/var/www/html/apps-cypress\\", \\"url\\" => \\"/apps2\\", \\"writeable\\" => true]\\n\\t],\\n];" > config/cypress.config.php\'')

	cy.exec('docker exec --user www-data nextcloud-cypress-tests-server tar -cf clean-state.tar apps-cypress config data')
})

beforeEach(() => {
	cy.exec('docker exec --user www-data nextcloud-cypress-tests-server rm -rf apps-cypress config data')
	cy.exec('docker exec --user www-data nextcloud-cypress-tests-server tar -xpf clean-state.tar')
})

---

Edit: Pushed another commit making all user modify tests less flaky as the users and groups are reset before each run. As suggested by the Cypress best practice guides.

[skjnldsv]

Ah, we talked abut this before with various other devs. I advocated for the usage of data-cy-xxxx and seemed that other devs agreed too after a some discussion. This allow to scope the elements.

I know what the best-practices says, but I think they do not have our overall ecosystem, so things differs a bit :)

The data attribute value should be used when it have a real changing value. Like data-cy-userslist-user="admin". Otherwise it makes really weird to target a specific element.
I also prefer to have more unique data attributes. The fact that you can grep all [data-cy="files-list-row"] makes no sense imho.
But [data-cy-files-list-row] and [data-cy-files-list-row-fileid="12345"] does :)

Any thoughts or counter arguments?
cc @artonge @max-nextcloud @Pytal as I recall you were in that discussion before 😉

[susnux]

For me it makes no difference if I use data-testid or data-cy both a pretty common and standard, so I am fine with both.

The fact that you can grep all [data-cy="files-list-row"] makes no sense imho.
But [data-cy-files-list-row] and [data-cy-files-list-row-fileid="12345"] does :)

Well for me not that much 😅
data-cy="files-list-row" for me this is like the id (without its singleton limitation), so it says "Select all elements that are file list rows". While data-cy-files-list-row-fileid="12345" says "get the file list row that for file ID 12345".

So my problem with data-cy-files-list-row vs data-cy="files-list-row" is that there is no consistent naming for identifying elements, you always need to use a specific attribute rather that a specific attribute ID.
This feels wrong in a HTML context.

But this is not about feels, so I have no problem to stick with one notation for all of our code and if it was decided to go with data-cy-some-identifier as boolean attribute then I am fine with it. As there is no technical downside 😃

Need some time to adjust this PR then for data-cy and we should probably add this to our development guides :)

[susnux]

TL;DR: Will take some time this week to adjust the test selectors 😉

[skjnldsv]

data-cy="files-list-row" for me this is like the id (without its singleton limitation), so it says "Select all elements that are file list rows". While data-cy-files-list-row-fileid="12345" says "get the file list row that for file ID 12345".

If you want all the lines, then you can also do [data-cy-files-list-row-fileid], without the id.
That's the benefit of scoped selectors. And with [data-cy="files-list-row"], you cannot select by Ids with that syntax.
Or with something like [data-cy="files-list-row-123456"], but I find it incredibly messy 🤷

[skjnldsv]

Exactly, previously hardcoded "jane doe" was used, but that might lead to inconsistent state.
Now random users are created, so at least the user does not exists and we start with a fresh profile.

Ah, I read it as the opposite 🙈
I thought you meant we should go for hardcoded users with Cypress 😉

[netlify]

✅ Deploy Preview for nextcloud-server ready!

Name	Link
🔨 Latest commit	6a8756c
🔍 Latest deploy log	https://app.netlify.com/sites/nextcloud-server/deploys/6538d0c3abb96d000895ffdd
😎 Deploy Preview	https://deploy-preview-41021--nextcloud-server.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[susnux]

Cypress error caused by theming, so will do a follow up fixing theming tests.