RFC: Added true e2e test suite #23479
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
took a quick glance, just shared couple of ideas for future tests, nice that you are exploring switching these to typescript! it's a more natural place for them.
One thing that caught little bit my eye is, to be cautious to and not overuse expectations in page objects.
Although it's there in playwright docs too, in it's original idea Page Object model is mostly for describing pages (it's sections and properties with locators etc).
Having expectations outside of POM, makes better separation of concerns, and easier reading of the tests and their intent. Through time it can get also difficult the manage. It also kinda breaks the ideal test -> that has 1 assert (not a very strict rule to obey, and hard to satisfy, but still one to strive for)
apps/e2e/README.md
Outdated
| This e2e test suite uses Playwright to automate a web browser and use the app as a real user would. The tests run independently from the system under test (Ghost), rather than hooking into Ghost to e.g. start and stop a development server. We should always be able to run this test suite by passing in a few configuration parameters via environment variables, like the base URL for the site we want to test. | ||
|
|
||
| ### Managing State | ||
| One of the key considerations in building a test suite such as this is how we manage state, which includes the MySQL database state and increasingly state that is kept in other services, i.e. Tinybird for analytics data. One approach we could use here is to hook into Ghost itself to i.e. truncate database tables and load new fixtures. As much as possible, we should avoid doing this, because it couples our tests too closely with the application code. Another, IMO better approach is to directly manage the state in whatever system it is stored in from the test suite itself. For example, rather than hooking into Ghost to truncate tables and load fixtures, we should build the infrastructure to allow the test suite to talk directly to MySQL and change the state itself. |
There was a problem hiding this comment.
Regarding managing state, we had the same challenge in postmarkapp.com. I agree that better approach is to manage state in whatever system it is stored, rather than through suite.
However, I would avoid talking to something like MySQL directly. I don't have an insight in how the infrastructure is setup, but instead of direct talk to services, you can build some form of API, that would hide that complexity from tests, and allow it to use a sort of a contract to talk with other services, even if they are changed later on.
There was a problem hiding this comment.
I like this a lot, thanks for the suggestion! Agree this will probably be a smoother route, especially for the "test in production" piece of this.
apps/e2e/README.md
Outdated
| A good example of this that is currently implemented is email: | ||
| - In the `admin.spec.ts` file, we login to Ghost Admin and go through the 2FA flow. To successfully operate this just as a user would, we need to be able to receive and inspect emails that are sent from Ghost. The `EmailService` in test/services supports two different modes for this: | ||
| 1. Live mode with MailSlurp: when testing against a live site on Pro, we need to use a real email address that Ghost can actually send an email to. MailSlurp allows us to create live, ephemeral inboxes where we can receive emails, then query for the received emails via API and inspect the messages. In the example of 2FA, we login with a Staff user whose email in Ghost is set to the address of one of these ephemeral inboxes. Ghost sends the OTP code to this email address over the actual internet, and the tests inspect the received email to extract the code, then input it into Ghost, exactly as a real user would do. | ||
| 2. Mock mode with MailHog: Operating in live mode gives us more confidence because we're making fewer assumptions in our tests, but that confidence comes at a cost: speed and stability. Sending emails from Ghost(Pro) to a real address on the open internet incurs additional network latency and consequently has more potential for flaky behavior. For that reason, we also support running with a mocked email service, MailHog. We run MailHog locally (via docker compose), point Ghost's email configuration to the MailHog service so the emails are sent from Ghost > MailHog over the local network, then we retrieve the emails MailHog receives by an API call over the local network. |
There was a problem hiding this comment.
one idea for mocking could be to use something like in Ruby vcr cassetes: https://github.com/vcr/vcr , not sure if javascript has something similar. (basically record external http requests, and replay them and use them as mock/stubs)
There was a problem hiding this comment.
Yeah I'd love to have some kind of record/replay ability, thanks for the suggestion. At the moment we're trying out MSW for the mocking layer. It doesn't have record/replay out of the box, but it can read from HAR files, so I suspect we can get something along those lines working 🤔
There was a problem hiding this comment.
e2e/src/page-objects/LoginPage.ts
Outdated
| this.defaultPassword = defaultPassword; | ||
| } | ||
|
|
||
| async goto() { |
There was a problem hiding this comment.
I might be suggesting this too early, but.. figured to share it :)
My assumption is that you will probably have this kind of methods across bunch of page objects, which makes them suited for a base/parent admin page and inheritance. Things like page url, visit to it, with default waits, maybe other helper methods like page resize, scroll to the end etc. They can be easily reused across pageobjects.
There was a problem hiding this comment.
Yes! This is definitely the dream, totally agree that we should do this from the start, so as we build out the test suite, we'll develop a full set of standardized Page Objects with the same core properties & methods
|
I would like to see this merged! However, I have additional requirements:
Thoughts? |
@ErisDS No disagreement, makes sense. Just thinking outloud: the main challenge to meeting those criteria is that these tests will depend, at least in part, on using docker compose in CI — ideally for the whole setup including Ghost, but we could also use local Ghost + backing services in compose setup. This setup would be a lighter lift initially, since we aren't currently building the docker image in CI. Does a hybrid approach sound okay to you, for the sake of getting this test suite merged and operational ASAP? My ideal "dream state" for our CI is that we replace the current setup step with a build step, which builds the docker image, then all the downstream jobs can run their tests against the built image, with whatever backing services are needed running in compose. However, doing that means pretty much a full rewrite of our CI processes, which I'd love to do but probably is out of scope for the sake of this specific change. |
|
I am in complete agreement on both the dream end state and intermediate steps 🙌 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #23479 +/- ##
==========================================
- Coverage 72.73% 72.73% -0.01%
==========================================
Files 1530 1530
Lines 111010 111010
Branches 13682 13682
==========================================
- Hits 80744 80741 -3
- Misses 29264 29265 +1
- Partials 1002 1004 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This reverts commit 00d09ab17e4c9a04e8bdbf9030b8c117baf6af8a.
…t database connection" This reverts commit b39d66049056602a25619d9efab3da81d91b1540.
This PR creates a brand new e2e test suite from scratch. The
apps/e2e/README.mdfile included in this PR is the best place to start to get acquainted with: