Skip to content

Rework sso e2e tests to use playwright vs. jest + playwright#3568

Open
humphd wants to merge 2 commits into
Seneca-CDOT:masterfrom
humphd:playwright-update
Open

Rework sso e2e tests to use playwright vs. jest + playwright#3568
humphd wants to merge 2 commits into
Seneca-CDOT:masterfrom
humphd:playwright-update

Conversation

@humphd

@humphd humphd commented Apr 28, 2022

Copy link
Copy Markdown
Contributor

Our e2e tests can be flaky. I decided to re-write them to get rid of Jest, and use Playwright directly. When we first started doing this, you had to run playwright via jest; but now that's not needed any more.

This uses turborepo to run the e2e tests. I still need to migrate the parser e2e tests over, which I'll do later. I also need to do some work on the CI side of this. But I wanted to get up what I'd done.

Playwright is such a joy to use, we should write front-end tests with it next.

@humphd humphd added the type: test Creation and development of test label Apr 28, 2022
@humphd humphd self-assigned this Apr 28, 2022
@humphd humphd requested review from DukeManh and menghif April 28, 2022 18:29
@gitpod-io

gitpod-io Bot commented Apr 28, 2022

Copy link
Copy Markdown

@humphd

humphd commented Apr 28, 2022

Copy link
Copy Markdown
Contributor Author

I'm not sure why Vercel's build is failing, is this related to me?

Failed to compile.
--
14:30:37.364 |  
14:30:37.365 | ./src/components/AboutFooter.tsx:80:8
14:30:37.365 | Type error: 'Box' cannot be used as a JSX component.
14:30:37.365 | Its element type 'ReactElement<any, any> \| Component<BoxProps, any, any> \| null' is not a valid JSX element.
14:30:37.365 | Type 'Component<BoxProps, any, any>' is not assignable to type 'Element \| ElementClass \| null'.
14:30:37.365 | Type 'Component<BoxProps, any, any>' is not assignable to type 'ElementClass'.
14:30:37.365 | The types returned by 'render()' are incompatible between these types.
14:30:37.365 | Type 'React.ReactNode' is not assignable to type 'import("/vercel/path1/node_modules/@types/react-transition-group/node_modules/@types/react/index").ReactNode'.
14:30:37.365 | Type '{}' is not assignable to type 'ReactNode'.
14:30:37.365 | Type '{}' is missing the following properties from type 'ReactPortal': key, children, type, props
14:30:37.365 |  
14:30:37.366 | 78 \|   return (
14:30:37.366 | 79 \|     <Grid container className={classes.root}>
14:30:37.366 | > 80 \|       <Box width={1} pb={5}>
14:30:37.366 | \|        ^
14:30:37.366 | 81 \|         {matches ? (
14:30:37.366 | 82 \|           <Grid container direction="row" justifyContent="space-between" alignItems="flex-start">
14:30:37.366 | 83 \|             <Grid container item xs={12} sm={3}>
14:30:37.393 | npm ERR! code ELIFECYCLE

@DukeManh DukeManh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is great.

The e2e timeout problem seems to persist if I try to run the tests right after spinning up the containers. The test takes more than 30s.

Comment thread pnpm-lock.yaml Outdated
'@babel/preset-typescript': 7.16.7_@babel+core@7.17.9
'@parcel/packager-ts': 2.4.1
'@senecacdot/eslint-config-telescope': 1.1.0_eslint@7.32.0
'@senecacdot/eslint-config-telescope': link:tools/eslint

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like you linked the local eslint config.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wonder why it did that? I just used pnpm install.

@@ -0,0 +1,32 @@
/* eslint-disable import/no-extraneous-dependencies */
const { devices } = require('@playwright/test');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't need wait-for-postgres anymore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if we needed it, now that we run migrations on the db before the tests? If those work, the db is up, right? I guess I could leave this in anyway, it won't hurt.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree, we could leave it

@DukeManh

Copy link
Copy Markdown
Contributor

I'm not sure why Vercel's build is failing, is this related to me?

Failed on #3569 too.

@DukeManh

DukeManh commented Apr 30, 2022

Copy link
Copy Markdown
Contributor

react/react#24304 (comment)

This error is caused by the recent incompatible version 18 of @types/react being installed. Some of the libraries we use depend on "@types/react": "*", most likely because it was copied over from packages published from the DefinitelyTyped repo.

The reason why only have this problem on Vercel is probably that we use pnpm locally and npm on Vercel? If so we could fix this by using pnpm on vercel, I think pnpm is supported https://vercel.com/changelog/projects-using-pnpm-can-now-be-deployed-with-zero-configuration.

If that doesn't fix it, we can overrides "@types/react" with pnpm like so:

"overrides": {
    "@types/react": "17.0.44",
  }

@humphd

humphd commented Apr 30, 2022

Copy link
Copy Markdown
Contributor Author

facebook/react#24304 (comment)

This error is caused by the recent incompatible version 18 of @types/react being installed. Some of the libraries we use depend on "@types/react": "*", most likely because it was copied over from packages published from the DefinitelyTyped repo.

The reason why only have this problem on Vercel is probably that we use pnpm locally and npm on Vercel? If so we could fix this by using pnpm on vercel, I think pnpm is supported https://vercel.com/changelog/projects-using-pnpm-can-now-be-deployed-with-zero-configuration.

If that doesn't fix it, we can overrides "@types/react" with pnpm like so:

"overrides": {
    "@types/react": "17.0.44",
  }

I've switched Vercel to use pnpm (you were correct, it was using npm). I rebuilt your PR for #3569 and it passed, so we're good. Thanks for investigating!

@humphd humphd force-pushed the playwright-update branch from c6947c9 to 77a8359 Compare May 2, 2022 15:16
@humphd humphd requested a review from DukeManh May 2, 2022 15:16
@humphd

humphd commented May 2, 2022

Copy link
Copy Markdown
Contributor Author

Rebased onto @DukeManh's test changes for SAML.

DukeManh
DukeManh previously approved these changes May 4, 2022
@humphd

humphd commented May 11, 2022

Copy link
Copy Markdown
Contributor Author

Looks like this is good to go.

DukeManh
DukeManh previously approved these changes May 12, 2022
@cindyorangis

Copy link
Copy Markdown
Contributor

Hi @humphd,

It looks like there's a conflict here. Can you resolve it for another round of reviews? Appreciate your work.

@manekenpix manekenpix force-pushed the master branch 6 times, most recently from e30dcf6 to 4570717 Compare December 3, 2022 19:33
@humphd

humphd commented Dec 3, 2022

Copy link
Copy Markdown
Contributor Author

@manekenpix I've reworked these tests, updated passport/playwright, etc, which I think might help reduce failures in other PRs.

I'll update to pick up your changes to the ci.yml

@humphd

humphd commented Dec 3, 2022

Copy link
Copy Markdown
Contributor Author

This should be working now, but it's not. I'm going to wait until master is fixed up and rebase/debug later.

In the meantime, this updates passport, playwright, supertest to newer in SSO.

@humphd

humphd commented Dec 4, 2022

Copy link
Copy Markdown
Contributor Author

I've rebased on current master, but for some reason, this hangs after the server starts, here:

@senecacdot/sso-service:e2e: [15:56:48.162] INFO (7205): Accepting Login/Logout for accepted origins
@senecacdot/sso-service:e2e:     allowedOrigins: [
@senecacdot/sso-service:e2e:       "http://localhost:8000",
@senecacdot/sso-service:e2e:       "http://localhost:8888"
@senecacdot/sso-service:e2e:     ]

It could be resource limitations, blocking the natural flow of the different containers. Everything works locally for me.

@humphd

humphd commented Jan 20, 2023

Copy link
Copy Markdown
Contributor Author

I spent a bunch more time on this, and I'm still stuck. I pass the tests with no issues locally, but my machine is really fast and has a ton of memory. I tried to re-create the VM running in GitHub Actions (2vCPU, 7GiB RAM) and I fail most of the tests. But! The tests do actually run. In GitHub Actions it seems to hang like it's waiting for them to start.

Part of me feels like the issue has to be resource contention in an underpowered VM (we have a lot of containers running); but our current e2e tests work, so I'm a bit baffled. Something about how this update runs with playwright is different, and I'm not sure (yet) what it is. I'll have to think more on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: test Creation and development of test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants