Use talkback proxy to mock e2e API requests #881

obulat · 2022-02-16T12:01:36Z

Fixes

Description

This PR adds a talkback proxy server that listens to the API requests during e2e testing. It saves the response data to test/tapes to be used on later requests.

This enables us to run e2e tests with real data, but without the internet access :)

I've also added a couple of pnpm scripts:

ci:e2e-test runs a bash script that is to be run by the CI. It uses bash because our CI runs on Linux only. It runs the proxy server, builds the app for production, starts it and runs all the e2e tests.
update-tapes removes all saved tapes, builds the server and runs the e2e tests, saving the responses to test/tapes.
There currently are no scripts that can be run with proxy server on Windows.
While testing on Windows I found that setting --hostname=0 will not allow to build the app on Windows, so I changed it to cross-platform 0.0.0.0 value.

Testing Instructions

Checkout the branch
pnpm i to install the new talkback dependency
run pnpm run ci:e2e-test. You should see the proxy and build and test logs, and finally, a message that the tests were successful.
If you delete a couple of files in test/tapes and try to run again; some tests that use those tapes will fail.
Now, you can run pnpm run update-tapes and see that new tapes are added, and all the e2e tests pass.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

pnpm-lock.yaml

package.json

sarayourfriend

Is it common not to save the tapes in the repository? If we don't then CI will never have them.

It would also be nice to have an "update-tapes" script that clears the tapes directory then runs the full e2e suite.

proxy.js

obulat · 2022-02-18T16:26:15Z

Is it common not to save the tapes in the repository? If we don't then CI will never have them.

Oops, I was sure I'd pushed the tapes :)

It would also be nice to have an "update-tapes" script that clears the tapes directory then runs the full e2e suite.
Great idea!

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

sarayourfriend · 2022-02-19T11:15:40Z

test/proxy.js

+const opts = {
+  host,
+  port: 3000,
+  path: './test/tapes',
+  record: talkback.Options.RecordMode.NEW,
+  debug: false,
+  name: 'Openverse e2e proxy',
+  tapeNameGenerator,
+}


Should we add the ignoreHeaders option to ignore user-agent, origin and referrer? For each one, here's why I think we shouldn't match tapes against them:

user-agent: This differs between platforms so even just running in CI or updating browser versions could cause a mismatch (though Chrome has frozen their UA string I think; but the tapes wouldn't match if we started testing multiple UAs like Firefox, Safari or mobile browsers)

origin: This will differ between environments, in particular the plan for the visual regression testing RFC, we'll run those e2e tests inside a docker container (we could run all playwright tests in docker if we wanted) and in that case the origin would either be host.docker.internal to allow for accessing the host's localhost; or if we use docker networking and run the frontend in docker as well as the e2e tests then it'd be the frontend container's hostname.

referrer: Same as origin.

We'd also want to include the two defaults of content-length and host I guess.

sarayourfriend · 2022-02-19T11:18:37Z

test/proxy.js

+  host,
+  port: 3000,
+  path: './test/tapes',
+  record: talkback.Options.RecordMode.NEW,


FWIW this appears to be the default value for record so we can leave it out if we keep it as NEW.

However, I'm wondering if we should actually change this to DISABLED and set the fallback mode to NOT_FOUND. I'm leaning in this direction because ideally we'd prevent any external request from hitting the actual API and get some noticeable errors happening if we've forgotten to add tapes for new routes, or if our matching rules aren't working as expected.

To make it easy to update tapes, it'd be convenient to look at process.argv for a --update-tapes param then only use RecordMode.NEW if that's present.

sarayourfriend · 2022-02-19T11:21:22Z

test/proxy.js

+ * tests less flaky due to changes in the API or API data.
+ */
+const talkback = require('talkback')
+const host = 'https://api.openverse.engineering/v1'


This comment #881 (comment) got resolved with this changing or explanation why we shouldn't. Was that just an oversight or was there a reason we need to keep the v1 prefix for the routes?

The API service uses the host URL to construct the request URL, so for search it would use ${host}/search/?=galah. If we don't add v1, then all the requests to the API go to the wrong URL (without v1), and the tests fail.

I think it's best to create an issue about it and either change the API service, adding /v1 to each request function individually, or change the Analytics server (why isn't it using /v1/ ?).

😱 I totally missed that that would be a problem when talkback actually has to forward the request 😬

I assumed the analytics endpoints aren't under v1 because they're technically not part of the open API of Openverse, they're just internal metrics gathering tools? 🤷

Agreed that the frontend should probably be making the requests itself.

I wonder if a stop-gap is to have two talkback servers running? Or is that overkill and we should just wait and fix it for real? 🤔

I assumed the analytics endpoints aren't under v1 because they're technically not part of the open API of Openverse, they're just internal metrics gathering tools? 🤷

that's right. it's a one line NGINX config change in the infra if we want to change this, btw!

Issue opened: #915

sarayourfriend · 2022-02-19T11:22:50Z

test/e2e/utils.js

-    mockImages(context),
-  ])
+const mockProviderApis = async (context) => {
+  await context.route('**.jamendo.com**', (route) => route.abort())


I kept meaning to ask about this, but why do we only mock the jamendo provider? Wouldn't we want to mock all providers to stop e2e from actually hitting them if we're mocking any of them at all?

I was mocking all the requests that end with jpg| svg | png before to make sure we stop hitting the providers. And it's not possible to mock the jamendo request by using an extension, because its URL doesn't have one. I think this should be another new issue: Configure mocking requests to providers for media.

Opened the issue: #914

dhruvkb

We should ignore the tapes from Prettier and ESLint. I'm not requesting changes because these are not blockers. Appreciate the grammar fixes to documentation.

dhruvkb · 2022-02-20T04:48:09Z

TESTING_GUIDELINES.md

+`pnpx install playwright`
+
+If you don't have the app running, start by running it in the dev mode:
+`pnpm run dev`


Triple backticks ``` would put the code snippets on their own line. That'll look better.

dhruvkb · 2022-02-20T04:49:36Z

test/proxy.js

+ * it tries to use the responses it previously saved in `/test/tapes` folder. If no
+ * response is found there, it:
+ * - by default, returns 'Not found'.
+ * - if you pass `--update-tapes` as a parameter, makes an actual request, and saves the response for


We would want this to run periodically so that we can ensure that the tapes don't diverge too much from the API responses.

Appreciate the grammar fixes to documentation.

It's all because of WebStorm complaining about them :)

Should we set up the weekly (monthly/daily?) action for this, @dhruvkb ?

.prettierignore

TESTING_GUIDELINES.md

Format

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

obulat · 2022-02-21T07:11:18Z

I had to write the e2e script for GitHub actions because concurrently was killing the proxy and that returned 1, which caused e2e tests to look as if they failed: https://github.com/WordPress/openverse-frontend/runs/5269787229?check_suite_focus=true

package.json

* Add basic e2e test for search navigation * Use the actual default filter data when updating filters from route * Add second test for search type button navigation * Remove unnecessary extra clonedeep * Patch content type tests until #881 is merged * Mix in provider filters to default filter data * Fix bad e2e test merge

# Conflicts: # package.json # test/e2e/content-types.spec.js

sarayourfriend

LGTM! There's one last thing though, I noticed that if for any reason talkback fails to start (for example if there's something already running on port 3000) then the e2e tests still run; ideally talkback failing to start would stop the whole test run.

Adding the typical fast-fail mechanisms won't work because we're using & to capture the PID, we'll need specific error handling for that case 😕

I don't want to block this PR any longer though, so feel free to create a fast-follow issue to correct that if you want.

Otherwise, this is awesome! I'm so excited to start using this 🎉

Note: also there are pnpm-lock.yaml updates that need to be committed from removing concurrently.

zackkrida · 2022-02-22T18:41:04Z

@obulat Apologies, could you update this PR with testing instructions? It looks like the commands in the PR body don't exist. I'd love to review this for you tomorrow.

obulat · 2022-02-22T19:54:20Z

@obulat Apologies, could you update this PR with testing instructions? It looks like the commands in the PR body don't exist. I'd love to review this for you tomorrow.

Sorry for not writing the testing instructions in the first place, @zackkrida . I've updated the issue description

zackkrida

This is very cool. Testing worked as-expected.

# Conflicts: # test/e2e/image-detail.spec.js # test/e2e/utils.js

obulat added 🟨 priority: medium Not blocking but should be addressed soon 🌟 goal: addition Addition of new feature 🤖 aspect: dx Concerns developers' experience with the codebase labels Feb 16, 2022

obulat self-assigned this Feb 16, 2022

obulat commented Feb 16, 2022

View reviewed changes

pnpm-lock.yaml Outdated Show resolved Hide resolved

sarayourfriend reviewed Feb 16, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

sarayourfriend added a commit that referenced this pull request Feb 16, 2022

Patch content type tests until #881 is merged

b74c53e

obulat added 2 commits February 17, 2022 08:18

Use talkback proxy to mock e2e API requests

2ec8181

Use concurrently to make e2e scripts cross-platform

749db43

obulat force-pushed the add/ssr_mocking_for_e2e branch from a49b2b5 to 749db43 Compare February 17, 2022 05:18

obulat marked this pull request as ready for review February 18, 2022 13:52

obulat requested a review from a team as a code owner February 18, 2022 13:52

obulat requested review from dhruvkb and stacimc February 18, 2022 13:52

Ensure tests can run cross-platform

dc87489

sarayourfriend suggested changes Feb 18, 2022

View reviewed changes

proxy.js Outdated Show resolved Hide resolved

sarayourfriend mentioned this pull request Feb 18, 2022

[RFC] Visual Regression Testing #891

Closed

9 tasks

obulat and others added 4 commits February 18, 2022 19:26

Update proxy.js

ea1f116

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

Save tapes

959fb02

Fix test url not matching because API url is replaced with proxy URL

0c7e257

Use concurrently for CI; move proxy to /test

ead82ef

sarayourfriend reviewed Feb 19, 2022

View reviewed changes

obulat added 4 commits February 20, 2022 07:18

Add new tapes

faf6bbc

Ignore headers and make recordMode adjustable

1495f5a

Update proxy and scripts

9859283

Add documentation

34aa5b0

dhruvkb reviewed Feb 20, 2022

View reviewed changes

sarayourfriend reviewed Feb 20, 2022

View reviewed changes

.prettierignore Show resolved Hide resolved

TESTING_GUIDELINES.md Outdated Show resolved Hide resolved

obulat and others added 3 commits February 21, 2022 08:39

Ensure that the proxy server is stopped after updating the tapes

97b001b

Format

Update TESTING_GUIDELINES.md

74ad5cd

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

Close the server if SIGTERM is received

c251af5

obulat force-pushed the add/ssr_mocking_for_e2e branch from b52ceb8 to c251af5 Compare February 21, 2022 05:39

obulat added 2 commits February 21, 2022 08:50

Close the server if SIGTERM is received

f80f4c8

Use bash script to prevent exit code 1 in CI

f4597e5

sarayourfriend added a commit that referenced this pull request Feb 21, 2022

Patch content type tests until #881 is merged

3189e17

sarayourfriend reviewed Feb 21, 2022

View reviewed changes

package.json Show resolved Hide resolved

sarayourfriend mentioned this pull request Feb 21, 2022

Fix browser back button handling in search pages #880

Merged

7 tasks

obulat added 2 commits February 21, 2022 21:19

Merge branch 'main' into add/ssr_mocking_for_e2e

6e54a7b

# Conflicts: # package.json # test/e2e/content-types.spec.js

Use the bash script for updating tests

b6fcf11

sarayourfriend approved these changes Feb 22, 2022

View reviewed changes

obulat mentioned this pull request Feb 22, 2023

Stop the e2e test suite if talkback proxy fails to start WordPress/openverse#613

Open

1 task

Add pnpm-lock file

a81646c

sarayourfriend mentioned this pull request Feb 22, 2022

Fix Audio e2e tests #558

Closed

1 task

zackkrida approved these changes Feb 22, 2022

View reviewed changes

obulat added 2 commits February 23, 2022 07:04

Merge branch 'main' into add/ssr_mocking_for_e2e

98963fa

Merge branch 'main' into add/ssr_mocking_for_e2e

c47c0bc

sarayourfriend mentioned this pull request Feb 28, 2022

Configure local visual regression testing #1017

Closed

6 tasks

obulat added 2 commits March 1, 2022 22:28

Merge branch 'main' into add/ssr_mocking_for_e2e

5a63303

# Conflicts: # test/e2e/image-detail.spec.js # test/e2e/utils.js

merge main

45ce41b

obulat merged commit ef84af5 into main Mar 1, 2022

obulat deleted the add/ssr_mocking_for_e2e branch March 1, 2022 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use talkback proxy to mock e2e API requests #881

Use talkback proxy to mock e2e API requests #881

obulat commented Feb 16, 2022 •

edited by zackkrida

Loading

sarayourfriend left a comment •

edited

Loading

obulat commented Feb 18, 2022

sarayourfriend Feb 19, 2022

sarayourfriend Feb 19, 2022

sarayourfriend Feb 19, 2022

obulat Feb 19, 2022

sarayourfriend Feb 19, 2022

zackkrida Feb 19, 2022

obulat Feb 20, 2022

sarayourfriend Feb 19, 2022

obulat Feb 19, 2022

obulat Feb 20, 2022

dhruvkb left a comment •

edited

Loading

dhruvkb Feb 20, 2022

dhruvkb Feb 20, 2022

obulat Feb 20, 2022

obulat Feb 20, 2022

obulat commented Feb 21, 2022

sarayourfriend left a comment •

edited

Loading

zackkrida commented Feb 22, 2022

obulat commented Feb 22, 2022

zackkrida left a comment

Use talkback proxy to mock e2e API requests #881

Use talkback proxy to mock e2e API requests #881

Conversation

obulat commented Feb 16, 2022 • edited by zackkrida Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend left a comment • edited Loading

Choose a reason for hiding this comment

obulat commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obulat commented Feb 21, 2022

sarayourfriend left a comment • edited Loading

Choose a reason for hiding this comment

zackkrida commented Feb 22, 2022

obulat commented Feb 22, 2022

zackkrida left a comment

Choose a reason for hiding this comment

obulat commented Feb 16, 2022 •

edited by zackkrida

Loading

sarayourfriend left a comment •

edited

Loading

dhruvkb left a comment •

edited

Loading

sarayourfriend left a comment •

edited

Loading