adding playwright VRT integration#86
Conversation
| */ | ||
| require('dotenv').config(); | ||
|
|
||
| const configuredBaseURL = (process.env.REMOTE_ENV_BASE_URL || '__DEFAULT_BASE_URL__') |
There was a problem hiding this comment.
It looks like this file reads process.env.REMOTE_ENV_BASE_URL to set the base URL. But the .env file the command generates writes CANDIDATE_URL and BASELINE_URL Mismatch?
| ``` | ||
|
|
||
| The `.env` file is added to `tests/playwright/.gitignore` by the init command. | ||
|
|
There was a problem hiding this comment.
I think we need to add a part here where we describe how to edit the common pages.
| @@ -0,0 +1,46 @@ | |||
| const { devices } = require('@playwright/test') | |||
There was a problem hiding this comment.
We're mixing 'require' and 'export' in the same file. We should pick one standard.
|
|
||
| const images = Array.from(document.querySelectorAll('img[loading="lazy"]')) | ||
|
|
||
| for (const image of images) { |
There was a problem hiding this comment.
FWIW, for loops can be slow sequence. Since this is JS we can fire these off in parallel and just await the work to be done.
| $this->taskExec($command)->run(); | ||
| } | ||
| else { | ||
| $this->taskExec($env . ' npm install')->dir($testsRoot)->run(); |
There was a problem hiding this comment.
I'm struggling to trace these commands back, including the init/install and reference commands. Some of them run in DDEV and other run on NPM. This worked for me because I already had playwright installed locally, but I'm not sure the logic is sound here. Maybe someone else can test from a clean install.
| $drupalRoot = $this->getDrupalRoot(); | ||
| $atkHome = getenv('ATK_HOME'); | ||
| if (!$atkHome) { | ||
| throw new AbortTasksException('ATK_HOME is not set in your environment. Add `export ATK_HOME=./tests/playwright` to your shell profile (PATH) and rerun.'); |
There was a problem hiding this comment.
I'm a little confused by this. We are telling them to set a path, but it has to be this specific path. Can we simply hardcode the path? If not, should we remove the hardcoded value in the 'run' command?
nJim
left a comment
There was a problem hiding this comment.
Approving the work, but added a number of comments to consider as future changes. In general I would say the direction is good. Backstop is aging and Playwright is a better long term fit. I also understand wanting to put this into Fire to help with adoption.
I had trouble getting this running locally, which surfaced how many moving parts there are and how hard failures are to debug. We have:
- composer required modules
- ATK to run scaffolding
- FIRE to copy templates, set ENVs, update files
- NPM to install packages and browsers
- CLI scripts that sometimes call other CLI script
I think this orchestration is going to make this very difficult to roll out. A proven template (tests/playwright) could simply be copied to every project with a readme to explain how to use it.
I think this effort looks like it's more about the automation of setup than the testing itself. The configuration does not exactly look correct (timeout, retry, possibly recording all traces.) Some variables are inconsistent (hardcoded vs configurable, ATK_HOME only works for init but not run). I think we can get this a little tighter before pushing for a every-site rollout. Or maybe try one site at a time and see what we learn.
Great work on moving this project forward. I think we can merge this as a big step towards the larger rollout.
| * @option $y Run the command with no interection required. | ||
| * | ||
| */ | ||
| public function vrtInit(ConsoleIO $io) { |
There was a problem hiding this comment.
vrt:init calls vrt:playwright:init feels strange on the command line. If we are depricating backstop, why even support new installations?
Functional Testing Instructions
Run these from the Drupal project root.
export ATK_HOME=./tests/playwright./vendor/fourkitchens/fire/bin/fire vrt:initExpected scripts include:
Expected: