sfbrigade / bats-server

Routed is an app to help ambulances direct non-critical patients to hospital emergency rooms with the most availability.
https://routedapp.org/
GNU Affero General Public License v3.0
18 stars 12 forks source link

304 write e2e tests for 2fa #306

Closed canjalal closed 1 year ago

canjalal commented 1 year ago

E2E tests now test for 2FA authentication for EMS and Healthcare users, having the tests run serially rather than default in parallel so that the 2FA authentication codes are picked up sequentially by MailCatcher. Typos for "authorization code" were also fixed.

I would like any feedback on how to DRY up this code and make the tests concise, testing what we need to test without duplication.

francisli commented 1 year ago

@canjalal looking at the last CircleCi log, the tests are now running, but failing. So the config looks fine, but the tests themselves need to be confirmed...?

francisli commented 1 year ago

@canjalal I've resolved the conflict on this branch with the latest on dev...

I also pulled down your branch, and ran the e2e tests locally, and I also get the errors seen on CircleCi. Do they actually pass when run on your computer?

canjalal commented 1 year ago

@canjalal I've resolved the conflict on this branch with the latest on dev...

I also pulled down your branch, and ran the e2e tests locally, and I also get the errors seen on CircleCi. Do they actually pass when run on your computer?

@francisli yes they do pass on my computer

francisli commented 1 year ago

@canjalal nevermind, my local test failures were related to the hardcoded mailcatcher hostname (localhost). since I'm running the tests inside the container, I've parameterized the tests so that they pull from the SMTP_HOST env var.

francisli commented 1 year ago

@canjalal after logging in to CircleCI and doing some experimentation, I'm pretty sure that the test failures are caused by timing issues.

That is, after the username/password is entered, the script immediately hits mailcatcher to get the latest message. However, in the CircleCI container, this is happening before the mail has been sent and received by the server. The script instead fetches an older email with a no-longer-valid auth code.

To verify this, I added a line of code to clear all mail from mailcatcher before the log in occurs, and sure enough, the test fails now because it fetches an empty messages list from mailcatcher.

So, we need to put in some sort of delay/wait on fetching messages from mailcatcher. And, it would probably be best to put a before/after hook to clear mailcatcher before/after every test.

canjalal commented 1 year ago

essages from mailcatcher. And, it would probably be best to put a before/after hook to clear mailcatcher before/after every te

@francisli are you able to clear Mailcatcher without having to shut down and restart it? I'm curious how you did it, because the documentation that I'm reading seems to suggest you have to do that, and not sure how to do that within Docker.

francisli commented 1 year ago

essages from mailcatcher. And, it would probably be best to put a before/after hook to clear mailcatcher before/after every te

@francisli are you able to clear Mailcatcher without having to shut down and restart it? I'm curious how you did it, because the documentation that I'm reading seems to suggest you have to do that, and not sure how to do that within Docker.

I actually went to the Mailcatcher source code to see the implementation of the REST API, and saw that you can issue a DELETE /messages http request to delete the inbox (this is probably the same call used when you hit the Clear button in the web UI): https://github.com/sj26/mailcatcher/blob/591eb16234e91aae4c3955ef9f0b33e30fbbd2dc/lib/mail_catcher/web/application.rb#L85

canjalal commented 1 year ago

Per previous comments, let's add hooks to clear mailcatcher before/after tests so we know each test starts from a clean slate.

Also, I notice you're hitting mailcatcher using fetch requests... you might also look into Playwright's classes for making API calls:

https://playwright.dev/docs/api-testing#sending-api-requests-from-ui-tests

Maybe there are some helpers in their implementation that might be useful? I.e. so that we can wait until mailcatcher returns a non-empty messages list...?

Alright, ready for second review, @francisli