-
-
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
ref(node): Run Node CI tests on Windows #4616
Conversation
fa27a25
to
eaab502
Compare
eaab502
to
05e5c7e
Compare
After these changes are in a good place, we'll have to update the CI to make these required. |
Still trying to work out why the build cache it not being found from the windows tests: |
diff --git a/packages/utils/test/is.test.ts b/packages/utils/test/is.test.ts
index 0b8f1a9d..5b06b7ad 100644
--- a/packages/utils/test/is.test.ts
+++ b/packages/utils/test/is.test.ts
@@ -1,6 +1,6 @@
-import { resolvedSyncPromise } from '../dist';
import { isDOMError, isDOMException, isError, isErrorEvent, isInstanceOf, isPrimitive, isThenable } from '../src/is';
import { supportsDOMError, supportsDOMException, supportsErrorEvent } from '../src/supports';
+import { resolvedSyncPromise } from '../src/syncpromise';
class SentryError extends Error {
public name: string;
diff --git a/packages/utils/test/syncpromise.test.ts b/packages/utils/test/syncpromise.test.ts
index 7c693c82..6164c666 100644
--- a/packages/utils/test/syncpromise.test.ts
+++ b/packages/utils/test/syncpromise.test.ts
@@ -1,5 +1,4 @@
-import { rejectedSyncPromise, resolvedSyncPromise } from '../dist';
-import { SyncPromise } from '../src/syncpromise';
+import { rejectedSyncPromise, resolvedSyncPromise, SyncPromise } from '../src/syncpromise';
describe('SyncPromise', () => {
test('simple', () => {
Needs to be applied, importing from the dist files is a code smell - I'll open the PR As for the Could we debug by running GHA locally with https://github.com/nektos/act? |
Made this change in #4631
I'd be fine with moving https://github.com/getsentry/sentry-javascript/blob/master/packages/utils/test/build.test.ts into a node script that runs in the We shouldn't be asserting on built files in jest unit tests. |
Thanks, will take another look! |
We shouldn't be asserting on built assets (dist/esm) in our unit tests. This helps unblock #4616
We shouldn't be asserting on built assets (dist/esm) in our unit tests. This helps unblock #4616
Opened #4658 for the build unit tests |
I managed to work around the fact that build caches cannot be shared between platforms by instead uploading and downloading artefacts. However, sharing an install cache between Windows agents does not work because the yarn workspace symlinks do not get copied. I think this is because unlike unix platforms, symlinks on Windows are not stored in the regular file system 🤦♂️. This means that on Windows we have to do a full I'm not convinced it's a great idea to add this into the CI build yet! |
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Closes #4613