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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Apr 28, 2022

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 menghif and DukeManh April 28, 2022 18:29
@gitpod-io
Copy link

gitpod-io bot commented Apr 28, 2022

@humphd
Copy link
Contributor Author

humphd commented Apr 28, 2022

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

Copy link
Contributor

@DukeManh DukeManh left a comment

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.

pnpm-lock.yaml Outdated
@@ -54,20 +50,16 @@ importers:
'@babel/preset-react': 7.16.7_@[email protected]
'@babel/preset-typescript': 7.16.7_@[email protected]
'@parcel/packager-ts': 2.4.1
'@senecacdot/eslint-config-telescope': [email protected]
'@senecacdot/eslint-config-telescope': link:tools/eslint
Copy link
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
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
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
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
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
Contributor

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

Failed on #3569 too.

@DukeManh
Copy link
Contributor

DukeManh commented Apr 30, 2022

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",
  }

@humphd
Copy link
Contributor Author

humphd commented Apr 30, 2022

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
Copy link
Contributor Author

humphd commented May 2, 2022

Rebased onto @DukeManh's test changes for SAML.

DukeManh
DukeManh previously approved these changes May 4, 2022
@humphd
Copy link
Contributor Author

humphd commented May 11, 2022

Looks like this is good to go.

DukeManh
DukeManh previously approved these changes May 12, 2022
@cindyledev
Copy link
Contributor

Hi @humphd,

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

@humphd
Copy link
Contributor Author

humphd commented Dec 3, 2022

@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
Copy link
Contributor Author

humphd commented Dec 3, 2022

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
Copy link
Contributor Author

humphd commented Dec 4, 2022

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
Copy link
Contributor Author

humphd commented Jan 20, 2023

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.

None yet

4 participants