Skip to content

test(e2e): Set up fake registry for E2E tests #5806

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

Merged
merged 28 commits into from
Sep 26, 2022
Merged

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Sep 22, 2022

Ref: #5506

This PR adds an initial setup for E2E tests by adding a script that launches a fake test registry (Verdaccio) and uploads our packages to that test registry.

Besides Verdaccio we also evaluated using yalc. I decided against yalc and in favor of Verdaccio because Verdaccio simulates better what users will do and therefore is what we want to test with these E2E tests:

  • Yalc writes any packages to file and links to them via a hard path in the package.json.
  • Verdaccio acts as an actual registry.

The Babel and React projects are also using Verdaccio to conduct their E2E tests, which gave me even more confidence in my choice. The Verdaccio docs even have a section on E2E tests: https://verdaccio.org/docs/e2e/

There are some nitty-gritty details in the run.ts script. I tried to explain them all via comments. While reviewing please point out if anything still looks fishy and I'll add some more comments!

In a next step, we should extend the test script to actually launch integration tests.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Cool - @priscilawebdev helps maintain Verdaccio 😄

Reminder, I'd like us to use a recipe system similar to: https://github.com/getsentry/sentry-electron/tree/master/test#end-to-end-tests

e2e-tests/run.sh Outdated
@@ -0,0 +1,27 @@
#!/bin/sh
set -e
Copy link
Member

@AbhiPrasad AbhiPrasad Sep 22, 2022

Choose a reason for hiding this comment

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

let's make this a node script so we stay windows friendly before we dive deeper into bash land

@lforst lforst mentioned this pull request Sep 23, 2022
9 tasks
@lforst lforst added this to the E2E Tests milestone Sep 23, 2022
@lforst lforst marked this pull request as ready for review September 26, 2022 09:09
@lforst lforst changed the title test(e2e): Set up test registry for E2E tests test(e2e): Set up fake registry for E2E tests Sep 26, 2022
packageTarballPaths.forEach(tarballPath => {
// For some reason the auth token must be in the .npmrc, for some reason the npm `--userconfig` flag doesn't always work,
// and for some reason the registry must be passed via `--registry` AND in the .npmrc because different npm versions
// apparently work different and we want it to work with different npm versions because of local development.
Copy link
Member

Choose a reason for hiding this comment

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

m: A single npm version should be enforced if people are using the node version set in package.json - let’s just remove that inconsistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if we wanna run the e2e tests with different node versions?

Copy link
Member

Choose a reason for hiding this comment

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

Feels weird though since this is essentially the setup code. Could we make a dockerfile and throw this in there? Thus we can run the actual tests with different node versions, but the setup will always be with the same one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah now I get it. Yeah I'll take a look. Dockerfile sounds good.

@priscilawebdev
Copy link
Member

Nice!!! 🚀

@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.47 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 60.23 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.09 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 53.16 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.85 KB (0%)
@sentry/browser - Webpack (minified) 64.49 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.87 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 44.78 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.92 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.36 KB (+0.01% 🔺)

// Publish built packages to the fake registry
packageTarballPaths.forEach(tarballPath => {
// `--userconfig` flag needs to be before `publish`
childProcess.execSync(`npm --userconfig ${__dirname}/test-registry.npmrc publish ${tarballPath}`, {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [file name](1).
// Publish built packages to the fake registry
packageTarballPaths.forEach(tarballPath => {
// `--userconfig` flag needs to be before `publish`
childProcess.execSync(`npm --userconfig ${__dirname}/test-registry.npmrc publish ${tarballPath}`, {
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to just use craft here (https://github.com/getsentry/craft/blob/master/src/targets/npm.ts#L160), but doesn't support custom user configs so we can stick with this.


// Run container that uploads our packages to fake registry
childProcess.execSync(
`docker run --rm -v ${repositoryRoot}:/sentry-javascript --network host ${PUBLISH_PACKAGES_DOCKER_IMAGE_NAME}`,

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
@lforst lforst merged commit bc1475a into master Sep 26, 2022
@lforst lforst deleted the lforst-e2e-test-registry branch September 26, 2022 14:18
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.

3 participants