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

feat(tests): Run Playwright tests using bundles. #4551

Merged
merged 23 commits into from
Feb 28, 2022
Merged

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Feb 11, 2022

Partly Resolves: #4546 (Resolves for Playwright, not for Karma integration tests)

This implementation heavily depends on hard-coded configuration, to filter modules or search paths, as the bundle names and paths don't have a pattern.

Built JavaScript modules (esm and dist) are used as webpack aliases, and imported by webpack, which I understand is how they're generally consumed.

Bundles are injected as <script>s into the generated HTML <head>.

@onurtemizkan onurtemizkan changed the title feat(test): Run Playwright tests using bundles. feat(tests): Run Playwright tests using bundles. Feb 11, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

size-limit report

Path Base Size (ab73aa8) Current Size Change
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.77 KB 19.77 KB 0%
@sentry/browser - ES5 CDN Bundle (minified) 63.47 KB 63.47 KB 0%
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.44 KB 18.44 KB +0.02% 🔺
@sentry/browser - ES6 CDN Bundle (minified) 56.6 KB 56.6 KB 0%
@sentry/browser - Webpack (gzipped + minified) 22.2 KB 22.2 KB 0%
@sentry/browser - Webpack (minified) 76.36 KB 76.36 KB 0%
@sentry/react - Webpack (gzipped + minified) 22.23 KB 22.23 KB 0%
@sentry/nextjs Client - Webpack (gzipped + minified) 46.4 KB 46.4 KB 0%
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.23 KB 27.23 KB +0.01% 🔺

@onurtemizkan onurtemizkan marked this pull request as ready for review February 11, 2022 14:01
@AbhiPrasad
Copy link
Member

Resolves for Playwright, not for Karma integration tests

I think this is fine, as we want playwright to be where we focus on from now on.

@AbhiPrasad AbhiPrasad added this to the Release Stability milestone Feb 11, 2022
@onurtemizkan onurtemizkan marked this pull request as draft February 11, 2022 14:35
@onurtemizkan
Copy link
Collaborator Author

Found double-importing issue when testing bundles. Will update in a bit.

@onurtemizkan
Copy link
Collaborator Author

onurtemizkan commented Feb 11, 2022

Could not avoid updating 30 templates after all. :/
I'll look for ways to lighten this configuration a bit, but this version works.

@onurtemizkan onurtemizkan marked this pull request as ready for review February 11, 2022 19:31
Copy link
Member

@lobsterkatie lobsterkatie 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 a great start, thank you!

I left a few comments below, but I think the biggest thing right now is to address the ES6/tracing bundle question. See #4551 (comment).

packages/integration-tests/README.md Outdated Show resolved Hide resolved
packages/integration-tests/utils/generatePage.ts Outdated Show resolved Hide resolved
packages/integration-tests/utils/generatePage.ts Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
onurtemizkan and others added 2 commits February 14, 2022 10:31
Co-authored-by: Katie Byers <lobsterkatie@gmail.com>
@onurtemizkan
Copy link
Collaborator Author

@lobsterkatie, @AbhiPrasad

I ended up implementing a tiny webpack plugin to detect which SDKs are imported by init.js and inject a bundle accordingly.

Also switched from handlebars templates to plain HTML templates and make HTMLWebpackPlugin to inject:

  1. @sentry/browser or @sentry/tracing bundle [optional]
  2. Sentry initialization script
  3. Test subject script

to the end of <body> with the given order.

Previously, we were adding init script to <head> and subject to <body>, but that doesn't seem needed with the current set of tests (unless there is a case where we need that, you might think of). So I could also delete those duplicate standard templates, switching to a global default.

Also added another env variable PW_TRACING_ONLY to only use @sentry/tracing bundles for all tests, including ones that don't need tracing. Added tracing-only jobs for all bundles on CI.

@AbhiPrasad
Copy link
Member

Hey, you'll have to update from master since we fixed some test failures!

We'll be taking a look at this soon to review, thanks for all the help @onurtemizkan!

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.

@lobsterkatie you think we can merge this in and add the es6 tracing bundle afterwards?

packages/integration-tests/README.md Outdated Show resolved Hide resolved
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

I like the plugin!

I left a few more comments, mostly just trying to make it clearer what various parts of the code mean, but overall this seems like it's in good shape! My only other question is what the empty files do. Will they ever be used, or are they just placeholders?

As for ES6 tracing, @AbhiPrasad, sure, I'm fine with merging this and adding the ES6 tracing bundle afterwards. The way this is set up now doesn't necessitate it the way the old setup did.

packages/integration-tests/README.md Outdated Show resolved Hide resolved
packages/integration-tests/utils/generatePlugin.ts Outdated Show resolved Hide resolved
packages/integration-tests/utils/generatePlugin.ts Outdated Show resolved Hide resolved
packages/integration-tests/utils/generatePlugin.ts Outdated Show resolved Hide resolved
packages/integration-tests/utils/generatePlugin.ts Outdated Show resolved Hide resolved
packages/integration-tests/utils/generatePlugin.ts Outdated Show resolved Hide resolved
packages/integration-tests/utils/generatePlugin.ts Outdated Show resolved Hide resolved
packages/integration-tests/utils/fixtures.ts Outdated Show resolved Hide resolved
@onurtemizkan
Copy link
Collaborator Author

Thanks for the review @lobsterkatie!

Updated 👍

My only other question is what the empty files do. Will they ever be used, or are they just placeholders?

They were just placeholders, but not required with the current set of tests, so removed them.

Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

One small comment fix, but otherwise this looks good to go!

Thanks, @onurtemizkan!

packages/integration-tests/utils/generatePlugin.ts Outdated Show resolved Hide resolved
Co-authored-by: Katie Byers <lobsterkatie@gmail.com>
@AbhiPrasad AbhiPrasad enabled auto-merge (squash) February 28, 2022 16:57
@AbhiPrasad
Copy link
Member

We might have to remove Browser Playwright Tests as a required check.

@AbhiPrasad AbhiPrasad disabled auto-merge February 28, 2022 18:24
@AbhiPrasad AbhiPrasad merged commit 0a4f278 into master Feb 28, 2022
@AbhiPrasad AbhiPrasad deleted the onur/test-bundles branch February 28, 2022 18:24
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.

Allow integration tests to run against built code
3 participants