Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add playwright create/login test for all db #4369

Closed
wants to merge 2 commits into from

Conversation

Timshel
Copy link
Contributor

@Timshel Timshel commented Feb 20, 2024

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.

Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Timshel
Copy link
Contributor Author

Timshel commented Feb 20, 2024

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 :(

test/scenarios/README.md Outdated Show resolved Hide resolved
test/scenarios/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@stefan0xC stefan0xC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😏

test/scenarios/README.md Outdated Show resolved Hide resolved
test/scenarios/tests/login-common.ts Outdated Show resolved Hide resolved
test/scenarios/test.env Outdated Show resolved Hide resolved
test/scenarios/README.md Outdated Show resolved Hide resolved
test/scenarios/global-setup.ts Outdated Show resolved Hide resolved
test/scenarios/global-setup.ts Outdated Show resolved Hide resolved
test/scenarios/global-setup.ts Outdated Show resolved Hide resolved
test/scenarios/global-utils.ts Outdated Show resolved Hide resolved
test/scenarios/tests/login_mariadb.spec.ts Outdated Show resolved Hide resolved
test/scenarios/tests/login_postgres.spec.ts Outdated Show resolved Hide resolved
test/scenarios/global-utils.ts Outdated Show resolved Hide resolved
test/scenarios/global-setup.ts Outdated Show resolved Hide resolved
test/scenarios/global-setup.ts Outdated Show resolved Hide resolved
test/scenarios/playwright.config.ts Outdated Show resolved Hide resolved
@Timshel
Copy link
Contributor Author

Timshel commented Feb 21, 2024

@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.

  1. 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.

  1. 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.

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

Yep sorry some stragglers.

@stefan0xC
Copy link
Contributor

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...

  1. 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
Copy link
Contributor Author

Timshel commented Feb 21, 2024

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
Copy link
Contributor

stefan0xC commented Feb 21, 2024

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
Copy link
Contributor Author

Timshel commented Feb 21, 2024

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
Copy link
Contributor Author

Timshel commented Feb 21, 2024

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
Copy link
Contributor Author

Timshel commented Mar 12, 2024

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
Copy link
Contributor Author

Timshel commented May 13, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

Timshel commented May 13, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

Timshel commented May 14, 2024

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).

@Timshel Timshel force-pushed the feature/playwright branch 2 times, most recently from 6814b52 to 0cfa462 Compare July 6, 2024 20:39
@Timshel
Copy link
Contributor Author

Timshel commented Jul 6, 2024

Hey

Worked on it a bit, wanted to add the ability to run against a non released version of bw_web_builds.
Made it able to run the docker build and extract the web-vault of a commit from a configurable bw_web_builds project.
But the Playwright setup was becoming even more janky :(.

So switched to using docker-compose to handle the build, this make it way cleaner but can't make it to handle conditional dependencies.

So running tests for postgres looks something like this

export DOCKER_BUILDKIT=1
docker-compose --env-file test.env --profile postgres  up -d Postgres
docker-compose --env-file test.env run Playwright test --project=postgres

To run with a custom version of the web-vault:

export DOCKER_BUILDKIT=1
export PW_WV_REPO_URL=https://github.com/stefan0xC/bw_web_builds
export PW_WV_COMMIT_HASH=c5b5279a8478385cee66a8cdfc804690cffe0315
docker-compose --env-file test.env up --build WebVault
docker-compose --env-file test.env run Playwright

Part of the handling of dependencies could be hidden/improved with a script.

@Timshel
Copy link
Contributor Author

Timshel commented Jul 8, 2024

Added docker-compose control in Playwright so to run all tests:

export DOCKER_BUILDKIT=1
docker-compose --env-file test.env run Playwright

@Timshel Timshel force-pushed the feature/playwright branch 2 times, most recently from 1d857a7 to 391e7e2 Compare July 10, 2024 18:34
@Timshel Timshel force-pushed the feature/playwright branch 2 times, most recently from 55601d1 to dfa891e Compare July 26, 2024 16:56
@Timshel
Copy link
Contributor Author

Timshel commented Jul 26, 2024

Added integration with https://github.com/maildev/maildev in the tests to be able to check the mail sent.
This will allow to test things like 2FA mail authentication and organization invitation.

@Timshel
Copy link
Contributor Author

Timshel commented Aug 20, 2024

Added some Organization invitation tests and switched to the compose plugin instead of docker-compose.
This now depends on :

@Timshel Timshel force-pushed the feature/playwright branch 2 times, most recently from c64242e to 546143f Compare August 23, 2024 14:02
@Timshel
Copy link
Contributor Author

Timshel commented Sep 5, 2024

Rebased on main since both previously PR are merged :).
Added some email 2FA tests :).

@Timshel Timshel closed this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants