Add playwright create/login test for all db

[Timshel]

Hey

So I extracted the Playwright tests I wrote for the SSO PR since it could be reviewed independently.

This allow to test account creation and login on all three databases.

The setup phase might be a bit verbose but I didn´t want to introduce another dependency to handle the building.
Structure could be a bit more flat but in the SSO PR it include an additional docker-compose to run Keycloak.

[williamdes]

Great idea to extract the tests.
Are they run by any CI ?
Should lines be added to .github/* workflow(s) ?

[Timshel]

Since it's using docker to run MaridaDb and Postgres no idea how easy it is to integrate.
Additionally the SSO version use docker-compose :(

[stefan0xC]

Tanks for the PR and the initiative. Adding a way to do some form of system testing has been on my personal todo list for a long time but I had no time to really dive into it. (I was evaluating the Rust-based options like thirtyfour but I don't think it's there yet.)

1. I'm not sure I'm a fan of the test/scenarios subfolder. I think it would make more sense to add it in the root directory of the project (so that testing becomes more important and not hidden away) and adding support for CI would be great too. Which is why I've also suggested some changes that would be important if we decide to make testing more of a first class citizen. (But this can be done at a later time too probably, so maybe it's enough to rename it as playwright directory.)

2. I've not looked into playwright enough to suggest concrete changes but I think that fundamentally it's not configured correctly (i.e. instead of browsers we should use the databases as projects because vaultwarden is fundamentally a backend project). And also I'd like to have the option to only run sqlite tests since running the containers for the other databases takes a long time and most of the time I'd be developing against the sqlite database. (I mean so that I can run npx playwright test --project=sqlite to speed up the tests.)

3. The setup is currently not that useful for developing Vaultwarden, so this would have to be adapted to our needs. (And also for CI.) But I think it's a great proof of concept.

4. One change I'd also like to see is the removal of the SSO specific parts because I think those should be added separately.

5. Lastly (but that's just a nit), the project is called Vaultwarden not VaultWarden. 😏

[Timshel]

@stefan0xC hey first thanks for the review :).

1. I'm not sure I'm a fan of the test/scenarios subfolder.

Unsure if you mean to move it to /scenarios ? as I mentioned in SSO PR there is additional logic for SSO with a docker-compose for keycloak that can be used to test locally and for scenarios. It's living in /test/oidc/keycloak.

2. on the databases

At first I wanted just a quick way to test migrations (made a broken one recently). But the projects feature look interesting will check it.

3. The setup is currently not that useful for developing Vaultwarden

I'm unsure what you mean by that, but yes they are integration tests not made for developing more for checking non regression of the core feature of the application.

4. One change I'd also like to see is the removal of the SSO

Yep sorry some stragglers.

[stefan0xC]

Unsure if you mean to move it to /scenarios ? as I mentioned in SSO PR there is additional logic for SSO with a docker-compose for keycloak that can be used to test locally and for scenarios. It's living in /test/oidc/keycloak.

I'd move it to /playwright for now (to keep the non-Rust stuff outside the root directory). I've not looked at your repository but it sounds to me like you are duplicating some setup stuff that should not be necessary.

Setup wise the stuff specific for SSO could be added to a separate project (that does a SSO specific setup). I've not thought about the folder layout but if we think long term it would make sense to me that tests specific to some kind of configuration that might require additional setup (i.e. if mail is required or not) should be possible and if we divide the stuff by subfolders (or have some naming convention) we can use testMatch and testIgnore to filter for the different test scenarios...

3. The setup is currently not that useful for developing Vaultwarden

I'm unsure what you mean by that, but yes they are integration tests not made for developing more for checking non regression of the core feature of the application.

I meant in context with my other comments in regards to what assumptions we make about the integration of the tests with the rest of the project and also if this should be part of the CI. For me this is more of an open discussion point and much of it should be addressed by the maintainers and/or could probably also be addressed later.

[Timshel]

I'd move it to /playwright for now

I know it's not practical but I'd like to settle on a layout which work with the SSO stuff since my goal with this PR is to facilitate review. If I still need to move everything in the SSO pr it defeat the purpose.

you are duplicating some setup stuff that should not be necessary

Can you give me some pointer on which stuff ?

Setup wise the stuff specific for SSO could be added to a separate project

Edit: There is a merged PR with wildcard support (but not yet released) for project all combination could be defined then the wildcard would make it more usable.

[stefan0xC]

I know it's not practical but I'd like to settle on a layout which work with the SSO stuff since my goal with this PR is to facilitate review. If I still need to move everything in the SSO pr it defeat the purpose.

Yeah, I get that but I think the keycloak stuff could just as well be added as a subfolder of the playwright directory (or if it makes more sense to the root directory of the project as well)? Since this is project written mainly in Rust I think that we might want to add a tests folder (for cargo test) eventually and it wouldn't do to have two similarly named folders (and keeping it as as separate folder named after the tool would be more in line with the docker folder). I mean, if we setup the playwright stuff to be more flexible I don't think that backporting the changes to your SSO PR would be that hard. (And like I said I'm not sure about the folder layout myself but I am definitely not a fan of looking into unnecessary subfolders.)

Can you give me some pointer on which stuff ?

Sorry, like I said I've not looked at your repository. I'm thinking about keeping your SSO changes separate for now so it will be easier to merge. Having a separate setup file for SSO specific changes (i.e. everything you've ripped out to make the web-vault compatible) can be seen as setup for a SSO project (and I was thinking that we might want do autostart keycloak like we do postgres/mariadb to test the changes - but I've not looked into it). I mean, ideally this shouldn't even be needed because the SSO stuff might use it's own official vaultwarden sso enabled web vault build which can be just pointed at with a separate WEB_VAULT_FOLDER.

[Timshel]

Ok I'll move it to /playwright.
The Keycloak docker-compose is also used to run a dev instance that's why I did not include it directly.

Do you think it would make sense to move it inside the docker folder ? something like docker/keycloak (maybe with a dockerignore to prevent polluting the docker build context).

[Timshel]

So moved everything and used Projects in the SSO PR and it's not too bad : https://github.com/Timshel/vaultwarden/blob/sso-support/playwright/playwright.config.ts#L38

[Timshel]

Hey,

Resolved the different discussions since I believe they are mostly linked to the question of keeping the separate temp folder to store the different built resources.

As I mentioned :

locally I want it to run separately from my dev setup and for it to be as independent as possible to minimize the need for manual setup and the risk of running against an invalid version (Was thinking of keeping a hash of the src folder to ensure that the compile vaultwarden is up to date with the src).

But of course if you are not interested in it working this way I'll make the change.

[Timshel]

Hey,

Where do we stand with this ? Is there still interest ? Should I change the way it works ?
I could expand the tests but not before this is merged.

This would probably be useful to help with faster merge/integration of the front-end.
For ex dani-garcia/bw_web_builds#160 is out of date again since v2024.4.3 was released.

[stefan0xC]

For ex dani-garcia/bw_web_builds#160 is out of date again since v2024.4.3 was released.

web-v2024.4.3 has not been released.

[Timshel]

A yes I was a bit fast when I checked the release, but my point is more that there is a struggle to keep pace with the clients release.

Edit:
Anyway my point was more that if the tests are merged then expanded it could help with the flow of releasing front-end version.
I don't know what is the current flow, but maybe for a given version if all tests are green then a front end build could be generated (maybe with a rc tag) then integrated in vaultwarden to be released under testing ?

But I was just trying to point where it could be useful. So to go back to the issue :

Is there still interest ? Should I change the way it works ?

[stefan0xC]

Well, I am interested but like I said I don't think that the current implementation is all that useful yet for how we do things and in my view we should discuss first how this would help us and where.

I don't know what is the current flow

The current flow is that when upstream releases a new web-vault, someone creates a PR in the bw_web_builds repository (where we keep track of our patches for the web-vault), this is then reviewed, merged and released. After the patched web-vault is released (and built and uploaded by CI), someone also has to create a PR to use the new web-vault in vaultwarden (which also needs to be merged as well to land in testing).

We certainly could improve this flow by adding playwright tests to the mix, as it would make it easier for making sure our patches stay compatible and reduce the burden of the maintainers but as I said that's a larger discussion because it might not make sense to add it to the vaultwarden repository when the patches to the web-vault are kept and developed in a separate repository.

Personally, I think that we probably could switch to a monorepo approach or alternatively have a separately managed repository for testing but that's something I'd like the input from @BlackDex and @dani-garcia on. I mean for me it would also make sense to rethink how vaultwarden is released and maybe work on multiple branches, e.g. a stable branch for hot fixes to the latest release, a testing branch for preparing the next release and an unstable one which tracks the latest changes and where the integration between the web-vault and vaultwarden (or new features) might not be guaranteed. (But that's something that the maintainers would have to decide.)

[Timshel]

Thinking on it in this current version it would be awkward to try to use it to validate new version of the front-end since I made it to rely on something already published. Of course, it could be changed but it would still mean using something outside the project.

At the same time I don't think moving this outside of Vaultwarden is a good idea since it's useful to ensure non regression from code modification in Vaultwarden and I found it quite useful to test things like database migration too (on all engines which is quite a pain manually otherwise).