-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
build: Move build related unit test into own job #4658
Conversation
We shouldn't be asserting on built assets (dist/esm) in our unit tests. This helps unblock #4616
size-limit report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. TIL that these tests even exist (both on the browser side and the utils side).
Two things:
-
The utils test is really a types test, to make sure we're not including node types where we shouldn't be. Could we call it
test/types/build.js
, or eventest/types/index.js
since there's unlikely to ever be more than one file in that folder? (I thinkpackage
is kind of ambiguous.) -
Out of the scope of this PR, but I wonder if the browser tests are even still necessary, now that we run all of our browser integration tests against all of our bundles.
Solid diligence on the review - I hastly put this together to unblock, but I maybe should have made that more clear. I'll make the changes for
We can easily migrate this part sentry-javascript/packages/browser/test/package/npm-build.js Lines 60 to 68 in 50a5d9e
sentry-javascript/packages/browser/test/package/npm-build.js Lines 55 to 58 in 50a5d9e
|
Oh, yeah. I'll admit that as far as the browser tests are concerned, I skimmed it, thought, "Oh, okay, it's just testing that the built bundle does the normal sentry stuff," and moved on. Now you’re the one doing the due diligence! At some point we might look back at any parts of that which aren't just testing that the built bundle does a thing (superseded by Onur's recent work, IMHO) to see if all of them are even still relevant, but not urgent. |
We shouldn't be asserting on built assets (dist/esm) in our unit tests.
This helps unblock #4616