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

test: Ensure browser-integration-tests cannot conflict due to build #13338

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Aug 13, 2024

This streamlines some stuff in our browser integration tests, to fix some flakiness (hopefully).

The biggest change is that instead of always building into dist for each test file, each test will now build into a random subfolder, e.g. dist/abc. This way, multiple tests in a single file will never conflict with each other.

Additionally it also streamlines some of the tests I encountered while looking at stuff, hopefully reducing flakes further.

Closes #13321

@mydea mydea self-assigned this Aug 13, 2024
@mydea mydea requested a review from a team as a code owner August 13, 2024 08:34
Copy link
Contributor

github-actions bot commented Aug 13, 2024

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path Size % Change Change
@sentry/browser 22.5 KB - -
@sentry/browser (incl. Tracing) 34.85 KB - -
@sentry/browser (incl. Tracing, Replay) 71.19 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 64.45 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 75.53 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 88.22 KB +0.07% +59 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.06 KB +0.08% +65 B 🔺
@sentry/browser (incl. metrics) 26.81 KB - -
@sentry/browser (incl. Feedback) 39.53 KB +0.17% +68 B 🔺
@sentry/browser (incl. sendFeedback) 27.13 KB - -
@sentry/browser (incl. FeedbackAsync) 31.84 KB +0.17% +55 B 🔺
@sentry/react 25.26 KB - -
@sentry/react (incl. Tracing) 37.83 KB - -
@sentry/vue 26.65 KB - -
@sentry/vue (incl. Tracing) 36.67 KB - -
@sentry/svelte 22.64 KB - -
CDN Bundle 23.75 KB +0.07% +16 B 🔺
CDN Bundle (incl. Tracing) 36.51 KB +0.05% +15 B 🔺
CDN Bundle (incl. Tracing, Replay) 70.83 KB +0.03% +17 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 76.1 KB +0.06% +44 B 🔺
CDN Bundle - uncompressed 69.64 KB +0.05% +31 B 🔺
CDN Bundle (incl. Tracing) - uncompressed 108.3 KB +0.03% +31 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 219.55 KB +0.02% +31 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 232.59 KB +0.08% +186 B 🔺
@sentry/nextjs (client) 37.59 KB - -
@sentry/sveltekit (client) 35.45 KB - -
@sentry/node 115.71 KB - -
@sentry/node - without tracing 90.1 KB -0.01% -1 B 🔽
@sentry/aws-serverless 99.51 KB - -

View base workflow run

@mydea mydea force-pushed the fn/flaky-test-streamline branch from 9e8f487 to 702d505 Compare August 13, 2024 09:31
Copy link
Member

@s1gr1d s1gr1d left a comment

Choose a reason for hiding this comment

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

Looks reasonable

Copy link
Member

@chargome chargome left a comment

Choose a reason for hiding this comment

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

LGTM, would it be worth to put the base url in a const though in case we need to update it again at some point?

@mydea
Copy link
Member Author

mydea commented Aug 13, 2024

LGTM, would it be worth to put the base url in a const though in case we need to update it again at some point?

the base url is really pretty arbitrary, in most cases it does not matter, IMHO it is just clearer to have it with a name like this vs some localhost name where it is not clear if this comes from somewhere important or not 😅

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

LGTM -- should we remove these try/catches so that if these error, they should break the build? (this shouldn't error anymore anyway)

@mydea mydea force-pushed the fn/flaky-test-streamline branch from 702d505 to d87049d Compare August 14, 2024 08:14
mydea added 2 commits August 14, 2024 11:59
This streamlines some stuff in our browser integration tests, to fix some flakiness (hopefully).

The biggest change is that instead of always building into `dist` for each test file, each test will now build into a random subfolder, e.g. `dist/abc`. This way, multiple tests in a single file will never conflict with each other.
@mydea mydea force-pushed the fn/flaky-test-streamline branch from d87049d to d35b73b Compare August 14, 2024 09:59
@mydea mydea merged commit 23980a0 into develop Aug 14, 2024
117 checks passed
@mydea mydea deleted the fn/flaky-test-streamline branch August 14, 2024 11:00
Zen-cronic pushed a commit to Zen-cronic/sentry-javascript that referenced this pull request Aug 26, 2024
…etsentry#13338)

This streamlines some stuff in our browser integration tests, to fix
some flakiness (hopefully).

The biggest change is that instead of always building into `dist` for
each test file, each test will now build into a random subfolder, e.g.
`dist/abc`. This way, multiple tests in a single file will never
conflict with each other.

Additionally it also streamlines some of the tests I encountered while
looking at stuff, hopefully reducing flakes further.

Closes getsentry#13321
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.

[Flaky CI]: replay/extendNetworkBreadcrumbs/xhr/captureResponseSize/test.ts
4 participants