set up e2e tests with playwright #941

ChristopherChudzicki · 2022-10-31T19:53:49Z

Pre-Flight checklist

Testing
- Code is tested
- Changes have been manually tested

What are the relevant tickets?

#919

What's this PR do?

This PR adds e2e testing via Playwright to OCW Hugo Themes.

There are a variety of other significant changes that I will try to call out in individual comments.

How should this be manually tested?

First: Run yarn install and read the readme: https://github.com/mitodl/ocw-hugo-themes/tree/cc/ci2/tests-e2e If you use VS Code, I highly recommend installing the Playwright extension. (See the Readme.).

Run yarn test:e2e. It should fail. Add the required environment variable. (Use value 3001; see set up e2e tests with playwright #941 (comment) ).
Run yarn test:e2e again. It should suceed but it will also be kinda slow.
Run yarn start:webpack. In a separate shell, run yarn test:e2e. It should be pretty fast. (Now Playwright is reusing the webpack server.)
Stick a .only on one of the tests, and run yarn test:e2e --debug. You can step through the test.
Run yarn start:course and yarn start:www, too. I changed some stuff with environment variables and scripts. Those should both still work. You could also try OCW_TEST_COURSE=different-from-value-in-env-file yarn start:course. That should work now, whereas previously it would not.

Where should the reviewer start?

I suggest starting with the readme, then reading some of the test files. The test files should more or less make sense without context.

Then look at all the other junk.

github-actions · 2022-10-31T19:59:34Z

Netlify Deployments:
www: https://ocw-hugo-themes-www-pr-941--ocw-next.netlify.app/
Course v1: https://ocw-hugo-themes-course-pr-941--ocw-next.netlify.app/
Course v2: https://ocw-hugo-themes-course-v2-pr-941--ocw-next.netlify.app/

github-actions · 2022-10-31T20:00:48Z

Lighthouse results:

results for https://ocw-hugo-themes-www-pr-941--ocw-next.netlify.app/:

Accessibility	Best Practices	Performance	Progressive Web App	SEO
100 💯	83 😄	55 😟	20 😱	79 🙂

results for https://ocw-hugo-themes-www-pr-941--ocw-next.netlify.app/search/:

Accessibility	Best Practices	Performance	Progressive Web App	SEO
93 🎉	92 🎉	70 🙂	30 😰	93 🎉

results for https://ocw-hugo-themes-course-v2-pr-941--ocw-next.netlify.app/:

Accessibility	Best Practices	Performance	Progressive Web App	SEO
74 🙂	100 💯	72 🙂	30 😰	79 🙂

ChristopherChudzicki · 2022-11-01T20:53:26Z

package_scripts/start_course.sh

@@ -3,31 +3,21 @@
 set -euo pipefail

 source package_scripts/common.sh
-load_env --require-dot-env


I got rid of load_env entirely. This function made it very hard to specify variables manually. It was less like "load env" and more like "forcefully override any environment variables that were already defined; replace them with the content of .env"

So now:

yarn start:course will by default use your env variables from .env

but OCW_TEST_COURSE=whatever yarn start:course will use OCW_TEST_COURSE as the value of that variable . I.e., now we can override them at the command line.

The downside is that running ./package_scripts/start_course.sh by itself in your shell will now probably fail, because...environment variables are missing. I'm sure there's some way to source your env vars if you wanted to, though. (In yarn scripts, we run it via dotenv-cli.)

Yea, I think we added this a while back because we were having trouble with dotenv. Like I mentioned above, I didn't realize that the dotenv-cli package was required and that you had to prefix any commands that needed the .env file with it. They don't mention it anywhere on the dotenv Github and it's kind of puzzling why the "CLI" component of that is a separate package. Either way, it works much better so yea it makes sense to get rid of this.

ChristopherChudzicki · 2022-11-01T20:56:59Z

base-theme/layouts/partials/webpack_url.html

@@ -1,9 +1,10 @@
+{{- $port := 3001 -}}


This $port should be $port := getenv "WEBPACK_PORT". All other instances of 3001 have been replaced with the environment variable.

The reason I left this one hard-coded is that in order to access the environment variable with hugo we need to whitelist it in the config, which requires a PR to ocw-hugo-projects.

I wanted CI to pass, so for now I have left this as instance as 3001.

This is the hugo projects PR: mitodl/ocw-hugo-projects#222, and https://github.com/mitodl/ocw-hugo-themes/tree/cc/ci3 branch of themes includes the required change.

ChristopherChudzicki · 2022-11-01T21:19:16Z

course-v2/layouts/partials/course_detail.html

+    <section class="course-detail-section">
      {{ partial "course_description.html" (dict "context" .) }}
-    </div>
+    </section>


⚠️ Site change! very minor

Changing div -> section here made seemed reasonable and made selecting "Course Info" section in Playwright a little easier. See testing-library/dom-testing-library#403 (comment) for a related issue. (I believe that issue is where I picked up the pattern of using .closest() for stuff like this. 🤷 )

ChristopherChudzicki · 2022-11-01T21:19:55Z

package.json

-    "start:www": "concurrently \"npm:start:webpack\" \"npm:start:www:hugo\" --kill-others",
-    "start:course": "concurrently \"npm:start:webpack\" \"npm:start:course:hugo\" --kill-others",
-    "start:fields": "concurrently \"npm:start:webpack\" \"npm:start:fields:hugo\" --kill-others",
+    "start:webpack": "dotenv -- webpack serve --config ./base-theme/assets/webpack/webpack.dev.ts --hot --host 0.0.0.0 --stats-children",


This is dotenv-cli. It loads env vars on the cli.

I feel like an idiot now because I didn't realize this package was required for NPM to pass env variables to package scripts. I thought all you needed install was the dotenv package and that would happen, but apparently I was mistaken.

I actually had never used this package before; I have used

node -r dotenv/config your_script.js

(See https://www.npmjs.com/package/dotenv#Preload) but that only works if you're calling node.

ChristopherChudzicki · 2022-11-01T21:29:44Z

base-theme/assets/webpack/webpack.dev.ts

+const env = envalid.cleanEnv(process.env, {
+  WEBPACK_PORT: envalid.port()
+})


This is https://www.npmjs.com/package/envalid. It's nice for (a) validating that env variables are set and providing good error messages if they are not, and (b) typescript. Since process.env.WHATEVER is string | undefined by default.

I've scattered a few envalid calls into the repo. I think one common usage of envalid is to have a single file that validates the environment variables and sets defaults and stuff. (Like settings.py in Django, I guess). I think that would be really nice, but we'd have to have some centralized js-based cli through which we call hugo (Rather than our current shell scripts). I briefly explored doing something like that with oclif, but it seemed like way too big a change for the moment.

Aside: oclif is cool. And it's built by Salesforce / it's what the heroku CLI is built upon. And while I like the idea of a centralized CLI for this repo, like django provides us elsewhere, I would not recommend oclif because https://npmtrends.com/commander-vs-oclif-vs-yargs

ChristopherChudzicki · 2022-11-01T21:33:28Z

package_scripts/build_all_courses.sh

@@ -3,7 +3,6 @@
 set -euo pipefail


@gumaerc Do you think we need this script anymore?

A while back I had been using this script to build all courses locally, and even before that it was actually being used in an EC2 box to deploy the whole site before we had ocw-studio and Concourse. Now that we have a full dev setup for ocw-studio that can build the whole site locally, I think we can safely remove this without consequence.

Unless someone wants it here, I think I'll drop that script in a separate PR.

ChristopherChudzicki · 2022-11-01T21:36:19Z

test-sites/ocw-ci-test-www/README.md

@@ -0,0 +1 @@
+# ocw-ci-test-www


I have not written very many tests for www yet, and this is the only file in the test site.

Really I included www mostly as a proof-of-concept for building / running tests against multiple sites using multiple hugo configs.

ChristopherChudzicki · 2022-11-01T21:39:28Z

tests-e2e/README.md

+
+### Is there a `--watch` mode for re-running tests?
+
+No. For VS Code users, a similar experience can be achieved with [Playwright for VS Code](https://marketplace.visualstudio.com/items?itemName=ms-playwright.playwright). Individual tests can be run from your editor with the click of a button.


For VS Code users, ... Individual tests can be run from your editor with the click of a button.

I swear I'm not just a shill for Microsoft.

pt2302 · 2022-11-02T17:31:15Z

During testing, found out we first need to run npx playwright install.

pt2302 · 2022-11-02T17:41:01Z

Runs in my tests with

ocw-hugo-themes/playwright.config.ts

Line 41 in f5ba169

    
           url:                 `http://localhost:${env.WEBPACK_PORT}/static/css/main.css`,

changed to http://0.0.0.0:${env.WEBPACK_PORT}/static/css/main.css.

pt2302

Looks good to me!

ChristopherChudzicki · 2022-11-03T19:48:28Z

playwright.config.ts

+  testDir: './tests-e2e',
+  /* Maximum time one test can run for. */
+  timeout: 30 * 1000,
+  expect:  {
+    timeout: 5000
+  },
+  fullyParallel: true,
+  forbidOnly:    !!process.env.CI,
+  retries:       process.env.CI ? 2 : 0,
+  workers:       process.env.CI ? 1 : undefined,
+  reporter:      'html',
+  use:           {
+    actionTimeout: 0,
+    trace:         'on-first-retry',
+  },


BTW: Most of these settings are what Playwright recommends. I created this config file via (https://playwright.dev/docs/intro#installing-playwright)

yarn create playwright

and these were the settings it gave me. My customizations were:

removing some comments because it seemed cluttery and TS docstrings / playwright docs have the same information.

removing the extra browsers (firefox, webkit)

customizing the webServer portion.

We could restore firefox + webkit. I'm indifferent. I'm not sure how useful the extra browsers would be at the moment; maybe if we were doing visual or very interactive tests.

ChristopherChudzicki · 2022-11-03T19:54:20Z

package.json

-  "packageManager": "[email protected]"
+  "packageManager": "[email protected]",
+  "devDependencies": {
+    "@playwright/test": "^1.27.1"


BTW: I mentioned on a zoom call once that there is playwright-testing-library akin to other libs we use like react-testing-library (all part of the Testing Library family).

I was initially using it, but decided to drop it. Playwright itself introduced some features inspired by testing-library, and the creator of playwright-testing-library says ¹:

Start out without playwright-testing-library and see how the new Testing Library API in 1.27 serves y

Footnotes

https://github.com/testing-library/playwright-testing-library/issues/558#issuecomment-1274195339w ↩

gumaerc

All of this looks great and is a good start to getting some actual testing done here. Everything works, I just have one request regarding the WEBPACK_PORT environment variable:

gumaerc · 2022-11-03T20:15:56Z

base-theme/assets/webpack/webpack.dev.ts

@@ -18,7 +23,7 @@ const devOverrides: Configuration = {
  devtool: "eval-source-map",

  devServer: {
-    port:    process.env.PORT || 3001,
+    port:    env.WEBPACK_PORT,


Why not env.WEBPACK_PORT || 3001? I get an error if I try to run yarn test:e2e without WEBPACK_PORT explicitly in my env. We default it to 3001 when running a site for dev, so why wouldn't we do that for tests?

We default it to 3001 when running a site for dev

If we merge this PR as-is, that is not longer true.

But, to be sure, having a required WEBPACK_PORT env variable also annoys me. But I do not think we currently have a good way to set the default that is accessible in JS+Hugo, and the current situation on main illustrates this.

Currently on main:

webpack.dev.ts uses process.env.PORT || 3001

but base-theme/layout/partials/webpack_url.html always uses 3001.

So you can see the inconsistency: if you have PORT env var set, webpack will respect it but hugo will not. 💥.

Here are some options that occur to me:

Hardcode 3001 everywhere. I don't really want to do this, but it doesn't actually seem terrible

Use an environment variable with no default. If it's not set, you get an error message. A little annoying.

Use an environment variable with inlined-defaults. This is the current situation, and IMO it is worse than (1) or (2), because of the possibility for inconsistency, as demonstrated by current maint branch.

Somehow have a "single source of truth" for environment variables that also allows setting defaults... I can think of two ways:

4A. Build a CLI... call it ocw. It would be like ./manage.py in django. We'd use it like:

yarn ocw --help yarn ocw test yarn ocw test-e2e yarn ocw build course <-- build a course; we'd use this in pipelines yarn ocw build course --help <-- get help info for all the flags/args to build yarn ocw serve course <-- serve a course in dev mode

where the ocw CLI would do something like load environment variables, set default values for some, provide useful error messages, have docstrings...like manage.py does.

This is appealing to me for a variety of reasons. Setting env variables is one. Another is it's a way to provide documentation without cluttering the README.

4B. Another option is to have .env and .env.defaults and

# replace this yarn dotenv <cmd> # with this: yarn dotenv -e .env -e .env.defaults <cmd>

I don't dislike this idea. But then do we have three files? (env, env.defaults, and env.sample)? Or do we just use env.sample in place of env.defaults? I'm not sure all the values in .env.sample are reasonable as defaults.

oops, submitted that too earlier...

In summary: 4A is my longer-term preference, but the approach of this PR is (2) which seems ok. Maybe 4B is good.

What do you think? (of these options, or of some other idea)

I actually think 3 is the best option specifically referring to the port. There aren't that many places where it's used to generate inconsistency, and I have never heard of software that spins up a service that binds to a port requiring you to specify said port without a default.

There aren't that many places where it's used to generate inconsistency,

But even with not many, we already have the inconsistency on main. Run PORT=3456 yarn start:course and observe that OCW does not load webpack assets.

Actually, I'm going to try (3)... if I can set all the js defaults in one place, that's good enough. If the env var is needed in Hugo, too, we'll have to set the default in two places. If we can keep it to two places, I think that's ok.

However it's accomplished, Node and Hugo don't require you to set a port when you run them and will run on a default port. We shouldn't require that it be set either, but allow it to be overridden. I'd rather hard code 3000 for Hugo and 3001 for Webpack than require the env variable to be set to spin up the server.

gumaerc · 2022-11-03T20:24:42Z

package.json

-    "start:www": "concurrently \"npm:start:webpack\" \"npm:start:www:hugo\" --kill-others",
-    "start:course": "concurrently \"npm:start:webpack\" \"npm:start:course:hugo\" --kill-others",
-    "start:fields": "concurrently \"npm:start:webpack\" \"npm:start:fields:hugo\" --kill-others",
+    "start:webpack": "dotenv -- webpack serve --config ./base-theme/assets/webpack/webpack.dev.ts --hot --host 0.0.0.0 --stats-children",


I feel like an idiot now because I didn't realize this package was required for NPM to pass env variables to package scripts. I thought all you needed install was the dotenv package and that would happen, but apparently I was mistaken.

gumaerc · 2022-11-03T20:28:54Z

package_scripts/start_course.sh

@@ -3,31 +3,21 @@
 set -euo pipefail

 source package_scripts/common.sh
-load_env --require-dot-env


Yea, I think we added this a while back because we were having trouble with dotenv. Like I mentioned above, I didn't realize that the dotenv-cli package was required and that you had to prefix any commands that needed the .env file with it. They don't mention it anywhere on the dotenv Github and it's kind of puzzling why the "CLI" component of that is a separate package. Either way, it works much better so yea it makes sense to get rid of this.

ChristopherChudzicki · 2022-11-04T13:57:20Z

package.json

-    "start:webpack": "dotenv -- webpack serve --config ./base-theme/assets/webpack/webpack.dev.ts --hot --host 0.0.0.0 --stats-children",
+    "start:webpack": "dotenv -- webpack serve --config ./base-theme/assets/webpack/webpack.dev.ts --hot --stats-children",


For posterity: @pt2302 and @ChristopherChudzicki on M1's reproducibly had an issue where:

all of ocw works fine on both node 16 and node 18, with one exception...

yarn test:e2e, which runs yarn start:webpack, did not work on node 18.

Removing --host 0.0.0.0 here fixes the issue for me.

I am not sure why we had --host 0.0.0.0 here in the first place. Based on https://stackoverflow.com/questions/35412137/how-to-get-access-to-webpack-dev-server-from-devices-in-local-network, I think this was probably set to enable testing OCW on phones.

But that doesn't seem to be working for me. Both my computer and my phone are on the same wifi network, and my phone cannot access http://localhost:3001/static/css/main.css.

So I think we should just remove this.

@gumaerc Do you know if there is any good reason to leave --host 0.0.0.0?

Noting that with this change, it runs for me on Apple Silicon using Node 18.14. @ChristopherChudzicki suggested that this might be due to a security setting on M1.

@ChristopherChudzicki Hitting http://localhost:3001 on your phone is going to try and access port 3001 on your phone, but your phone isn't running a web server. Binding to 0.0.0.0 means that you can hit the local IP address of your dev machine from any other device on your network. Say your dev machine has a local IP of 192.168.1.100. That means that if you run this with --host 0.0.0.0 then on your phone you can go to http://192.168.1.100:3001 and you will get your webpack instance.

@gumaerc Awesome, thank you for that explanation! Sounds like we do want 0.0.0.0. I've reverted the "replace 0.0.0.0 with localhost" for now.

ChristopherChudzicki

@gumaerc I believe that the most recent commit (c9cc0ee) has all changes since your last review. Aside from a rebase with no conflicts.

I went ahead and set a default port in a 4 places. I'm not super happy with this and I plan to make a small folllowup PR that reorganizes a few env variable things, but I'd rather not add even more env-reorganization to this PR.

Thanks again for your explanations about 0.0.0.0 (I reverted the 0.0.0.0 --> localhost change for now; will also address that in my incoming PR).

Anyway, try: start:www, start:course, and test:e2e without a WEBPACK_PORT explicitly defined. Should work.

gumaerc

👍 LGTM

ChristopherChudzicki force-pushed the cc/ci2 branch 2 times, most recently from a2d7d87 to 19703f6 Compare October 31, 2022 19:59

ChristopherChudzicki force-pushed the cc/ci2 branch 6 times, most recently from c67d525 to bc449de Compare November 1, 2022 21:32

ChristopherChudzicki commented Nov 1, 2022

View reviewed changes

ChristopherChudzicki force-pushed the cc/ci2 branch from bc449de to df0be02 Compare November 1, 2022 22:29

ChristopherChudzicki requested review from gumaerc and pt2302 November 1, 2022 23:47

ChristopherChudzicki added the needs review label Nov 1, 2022

ChristopherChudzicki force-pushed the cc/ci2 branch from df0be02 to f5ba169 Compare November 2, 2022 00:12

ChristopherChudzicki marked this pull request as ready for review November 2, 2022 00:36

pt2302 self-assigned this Nov 2, 2022

ChristopherChudzicki force-pushed the cc/ci2 branch from bbeb67b to 47c9006 Compare November 3, 2022 16:26

pt2302 approved these changes Nov 3, 2022

View reviewed changes

ChristopherChudzicki commented Nov 3, 2022

View reviewed changes

gumaerc requested changes Nov 3, 2022

View reviewed changes

ChristopherChudzicki commented Nov 4, 2022

View reviewed changes

ChristopherChudzicki added 4 commits November 4, 2022 14:14

set up e2e tests with playwright

2d51d43

add some example tests

40d2a18

remove last hardcoded WEBPACK_PORT

044594f

remove hardcoded branch

ce42882

specify node version

d6c25cf

ChristopherChudzicki force-pushed the cc/ci2 branch 3 times, most recently from dbc8a01 to ed29972 Compare November 4, 2022 18:31

set default ports for webpack

c9cc0ee

ChristopherChudzicki force-pushed the cc/ci2 branch from ed29972 to c9cc0ee Compare November 4, 2022 18:33

ChristopherChudzicki commented Nov 4, 2022

View reviewed changes

ChristopherChudzicki requested a review from gumaerc November 4, 2022 18:37

gumaerc approved these changes Nov 4, 2022

View reviewed changes

ChristopherChudzicki merged commit 66c63c7 into main Nov 5, 2022

This was referenced Nov 7, 2022

Remove some old env vars, scripts #963

Closed

Clean up env variables + make them all optional #964

Merged

This was referenced Nov 9, 2022

Release 1.77.0 #970

Closed

Release 1.76.4 #977

Merged

ChristopherChudzicki mentioned this pull request Dec 20, 2022

Test build failures #1018

Closed


		### Is there a `--watch` mode for re-running tests?

		No. For VS Code users, a similar experience can be achieved with [Playwright for VS Code](https://marketplace.visualstudio.com/items?itemName=ms-playwright.playwright). Individual tests can be run from your editor with the click of a button.

		"start:webpack": "dotenv -- webpack serve --config ./base-theme/assets/webpack/webpack.dev.ts --hot --host 0.0.0.0 --stats-children",
		"start:webpack": "dotenv -- webpack serve --config ./base-theme/assets/webpack/webpack.dev.ts --hot --stats-children",

set up e2e tests with playwright #941

set up e2e tests with playwright #941

Uh oh!

Conversation

ChristopherChudzicki commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Where should the reviewer start?

Uh oh!

github-actions bot commented Oct 31, 2022

Uh oh!

github-actions bot commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pt2302 commented Nov 2, 2022

Uh oh!

pt2302 commented Nov 2, 2022

Uh oh!

pt2302 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

gumaerc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki commented Oct 31, 2022 •

edited

Loading

github-actions bot commented Oct 31, 2022 •

edited

Loading

ChristopherChudzicki Nov 1, 2022 •

edited

Loading

ChristopherChudzicki Nov 1, 2022 •

edited

Loading

ChristopherChudzicki Nov 3, 2022 •

edited

Loading

ChristopherChudzicki Nov 3, 2022 •

edited

Loading

ChristopherChudzicki Nov 4, 2022 •

edited

Loading

ChristopherChudzicki left a comment •

edited

Loading