Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix end-to-end tests post Next.js migration #4346

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

jcmorrow
Copy link
Contributor

@jcmorrow jcmorrow commented Nov 6, 2021

Changes

  • In order to run either the mock tests or the end-to-end tests, we need a dev server running in CI. Previously, we would spawn that manually from inside of a script. Now, we start that from inside of the action that does the testing. This means that if you want to run the tests locally, you'll also need your local dev server running (which was already true).
  • Add the test/examples and test/scripts folder to public. This means that we will be able to run against preview branches as well.
  • Add a GitHub action for setting up Next.js with a cache, so we only have to copy one line everywhere, not an entire step. It would be nice if we could further refactor out the test stripes, because a lot of things are repeated 9 times.
  • Remove the dev-server.js file. If we're going to use next.js, then we should try and keep our config as out-of-the-box as possible, in order to allow us to stay up-to-date. The fact that the test folder has to be available on the development server for the tests to run at all seems like the real problem here. A good follow-on task would be to sort out the best way to handle those files in the test/dev environment.
  • Go back to node 14 for lock file. I didn't realize when I changed this that we were stuck on such an outdated node version. We should fix this ASAP, but until then, let's stick with what was working, just to minimize the factors that have gone into resolving all of the failing tests.
  • Log more from playwright tests. This could be annoying, maybe we should tune it, but it's nice to know what's going on in these tests, and we're only going to look at the output when something is failing anyways, so it may as well be verbose.
  • Disable the worst of the failing tests.

But the tests are still failing...

That's true! We still have flaky, intermittent tests. The goal of this PR was to get things to run, getting them to pass is a future us problem! I have been unable to get the mock tests to work to any extent on CI, although they do pass intermittently locally.

There are also tests (it doesn't seem to be consistent which ones) where the browser starts up and just never seems to navigate to the page properly. I haven't been able to figure out where it's getting stuck, but when tests time out now, it's usually for this reason.

Notes on GitHub Actions

I'm going to put these into a notion doc as well, but documenting here couldn't hurt. Three things that tripped me up on this PR:

  • any merge conflicts mean actions will not run at all. If you have a branch and it's unclear why actions aren't kicking off, make sure you check for and resolve merge conflicts.
  • When using actions that are type javascript your inputs get automatically turned into environment variables, which is convenient. However when using composite actions this won't happen. You have to manually pass the environment variables in with an expression.
  • The checkout action that we use all the time performs a strict clean of the directory that it's about to clone into! So, if you are trying to do some sort of caching or otherwise unloading assets into a dir that you also need to do git checkout XXXX in (with the checkout action), you either need to checkout first, or you can use [clean: false](https://github.com/actions/checkout#usage) I think, though I haven't actually tried this.

Fixes #4313

@vercel
Copy link

vercel bot commented Nov 6, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/recordreplay/devtools/AMTJVzZuzsBHc1dxKuM6DXKNxSJL
✅ Preview: https://devtools-git-fork-jcmorrow-jm-build-fork-recordreplay.vercel.app

@jcmorrow jcmorrow force-pushed the jm-build-fork branch 2 times, most recently from a95da19 to c23328c Compare November 6, 2021 03:25
@@ -170,7 +170,7 @@ export function useIsTeamDeveloper() {
console.error("Apollo error while getting the user's role", error);
}

return { isTeamDeveloper: data.recording.userRole !== "team-user", loading };
return { isTeamDeveloper: data?.recording.userRole !== "team-user", loading };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gosh - you're deep in the guts of devtools here

Copy link
Collaborator

@jasonLaster jasonLaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of good stuff!

// example: "doc_inspector_styles.html",
// script: "inspector-03.js",
// targets: ["gecko"],
// },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We should file issues for each of the disabled tests
  2. We should add a skip: true field to these tests so that the runner can Nag us to fix them. This is a really dangerous spot to be in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -475,6 +492,7 @@ async function createExampleBrowserRecording(url, target) {
}

if (failures.length) {
console.log({ timings });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@jasonLaster
Copy link
Collaborator

jasonLaster commented Nov 9, 2021

Even if this is one PR, it should be several commits for version control reasons

A simple rule of thumb would be

  1. Test example files
  2. src fixes
  3. helper changes
  4. harness changes

jcmorrow added a commit to jcmorrow/devtools that referenced this pull request Nov 9, 2021
These are all small fixes that came from replayio#4346 that either eliminate
possible console errors or eliminate linting/tsc errors.
jcmorrow added a commit to jcmorrow/devtools that referenced this pull request Nov 9, 2021
These are all small fixes that came from replayio#4346 that either eliminate
possible console errors or eliminate linting/tsc errors.
- In order to run either the mock tests or the end-to-end tests, we need
  a dev server running in CI. Previously, we would spawn that manually
  from inside of a script. Now, we start that from inside of the action
  that does the testing. This means that if you want to run the tests
  locally, you'll also need your local dev server running (which was
  already true).
- Add the `test/examples` and `test/scripts` folder to public. This
  means that we will be able to run against preview branches as well.
- Add a GitHub action for setting up Next.js with a cache, so we only
  have to copy one line everywhere, not an entire step. It would be nice
  if we could further refactor out the test stripes, because a lot of
  things are repeated 9 times.
- Remove the `dev-server.js` file. If we're going to use next.js, then
  we should try and keep our config as out-of-the-box as possible, in
  order to allow us to stay up-to-date. The fact that the `test` folder
  has to be available on the development server for the tests to run at
  all seems like the real problem here. A good follow-on task would be
  to sort out the best way to handle those files in the test/dev
  environment.
- Go back to node 14 for lock file. I didn't realize when I changed this
  that we were stuck on such an outdated node version. We should fix
  this ASAP, but until then, let's stick with what was working, just to
  minimize the factors that have gone into resolving all of the failing
  tests.
- Log more from playwright tests. This could be annoying, maybe we
  should tune it, but it's nice to know what's going on in these tests,
  and we're only going to look at the output when something is failing
  anyways, so it may as well be verbose.
- Disable the worst of the failing tests.

That's true! We still have flaky, intermittent tests. The goal of this
PR was to get things to *run*, getting them to pass is a future us
problem! I have been unable to get the mock tests to work to any extent
on CI, although they do pass intermittently locally.

There are also tests (it doesn't seem to be consistent which ones) where
the browser starts up and just never seems to navigate to the page
properly. I haven't been able to figure out where it's getting stuck,
but when tests time out now, it's usually for this reason.

I'm going to put these into a notion doc as well, but documenting here
couldn't hurt. Three things that tripped me up on this PR:

- *any* merge conflicts mean actions *will not run at all*. If you have
  a branch and it's unclear why actions aren't kicking off, make sure
  you check for and resolve merge conflicts.
- When using actions that are type `javascript` your `inputs` get
  automatically turned into environment variables, which is convenient.
  However **when using `composite` actions this won't happen**. You have
  to manually pass the environment variables in with an
  [expression](https://docs.github.com/en/actions/learn-github-actions/expressions).
- The `checkout` action that we use all the time performs a strict
  `clean` of the directory that it's about to clone into! So, if you are
  trying to do some sort of caching or otherwise unloading assets into a
  dir that you also need to do `git checkout XXXX` in (with the checkout
  action), you either need to checkout *first*, or you can use `[clean:
  false](https://github.com/actions/checkout#usage)` I think, though I
  haven't actually tried this.

Fixes replayio#4313
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants