Skip to content

ci: Split node & browser unit tests #6535

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 6 commits into from
Dec 15, 2022
Merged

Conversation

mydea
Copy link
Member

@mydea mydea commented Dec 14, 2022

This PR does two main things:

  • Streamline names of CI jobs for better readability (=put the most important stuff up front, as it is often truncated at the end)
  • Split unit tests into Browser & Node unit tests - so we only run tests that depend on node stuff in multiple node versions.

@mydea mydea self-assigned this Dec 14, 2022
@mydea mydea force-pushed the fn/unit-node-browser-tests branch 2 times, most recently from 0c8046b to 9ee6705 Compare December 15, 2022 08:36
@github-actions
Copy link
Contributor

github-actions bot commented Dec 15, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.78 KB (-0.02% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 61.27 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.57 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 54.8 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.34 KB (0%)
@sentry/browser - Webpack (minified) 66.48 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.36 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.57 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.75 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.19 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 41.89 KB (+0.02% 🔺)
@sentry/replay - Webpack (gzipped + minified) 38.12 KB (+0.02% 🔺)

@mydea mydea requested review from a team, Lms24 and lobsterkatie and removed request for a team December 15, 2022 08:58
@mydea mydea added the Dev: CI label Dec 15, 2022
@mydea mydea marked this pull request as ready for review December 15, 2022 08:58
@mydea mydea requested review from AbhiPrasad and lforst and removed request for lobsterkatie December 15, 2022 09:00
package.json Outdated
Comment on lines 29 to 30
"test-ci-browser": "TESTS_SKIP=node ts-node ./scripts/test.ts",
"test-ci-node": "TESTS_SKIP=browser ts-node ./scripts/test.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't remember: Does this work on windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, I guess we can use cross-env!

Copy link
Contributor

@lforst lforst left a comment

Choose a reason for hiding this comment

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

noice

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Generally, I think it's good to split up this huge unit test job. I'm just wondering if there's a downside to not testing browser packages with different node versions... Maybe we'd miss some bugs appearing in SSR scenarios? But I guess overall that's an edge case and we're testing NextJS and Remix anyway in the node jobs. So I'd say let's do it 👍

@mydea mydea force-pushed the fn/unit-node-browser-tests branch from bbe3f1e to 076b133 Compare December 15, 2022 15:57
@mydea mydea merged commit 16b7343 into master Dec 15, 2022
@mydea mydea deleted the fn/unit-node-browser-tests branch December 15, 2022 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants