test: Avoid race conditions with symlinks (#13498)

Noticed here: https://github.com/getsentry/sentry-javascript/actions/runs/10594383971/job/29358133582 that this was sometimes failing. While looking into this, we actually did unnecessary work here - we had two levels of symlinks. Now we simply have a single symlink, and since we have unique dirs now we can skip checking for existing files etc.
getsentry · Aug 28, 2024 · 5c15485 · 5c15485
1 parent 9e56c74
commit 5c15485
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 33 deletions.
diff --git a/dev-packages/browser-integration-tests/utils/generatePlugin.ts b/dev-packages/browser-integration-tests/utils/generatePlugin.ts
@@ -4,7 +4,7 @@ import type { Package } from '@sentry/types';
 import HtmlWebpackPlugin, { createHtmlTagObject } from 'html-webpack-plugin';
 import type { Compiler } from 'webpack';
 
-import { addStaticAsset, addStaticAssetSymlink } from './staticAssets';
+import { addStaticAsset, symlinkAsset } from './staticAssets';
 
 const LOADER_TEMPLATE = fs.readFileSync(path.join(__dirname, '../fixtures/loader.js'), 'utf-8');
 const PACKAGES_DIR = path.join(__dirname, '..', '..', '..', 'packages');
@@ -214,7 +214,10 @@ class SentryScenarioGenerationPlugin {
                 src: 'cdn.bundle.js',
               });
 
-          addStaticAssetSymlink(this.localOutPath, path.resolve(PACKAGES_DIR, bundleName, bundlePath), 'cdn.bundle.js');
+          symlinkAsset(
+            path.resolve(PACKAGES_DIR, bundleName, bundlePath),
+            path.join(this.localOutPath, 'cdn.bundle.js'),
+          );
 
           if (useLoader) {
             const loaderConfig = LOADER_CONFIGS[bundleKey];
@@ -245,14 +248,13 @@ class SentryScenarioGenerationPlugin {
               const fileName = `${integration}.bundle.js`;
 
               // We add the files, but not a script tag - they are lazy-loaded
-              addStaticAssetSymlink(
-                this.localOutPath,
+              symlinkAsset(
                 path.resolve(
                   PACKAGES_DIR,
                   'feedback',
                   BUNDLE_PATHS['feedback']?.[integrationBundleKey]?.replace('[INTEGRATION_NAME]', integration) || '',
                 ),
-                fileName,
+                path.join(this.localOutPath, fileName),
               );
             });
           }
@@ -262,26 +264,23 @@ class SentryScenarioGenerationPlugin {
           if (baseIntegrationFileName) {
             this.requiredIntegrations.forEach(integration => {
               const fileName = `${integration}.bundle.js`;
-              addStaticAssetSymlink(
-                this.localOutPath,
+              symlinkAsset(
                 path.resolve(
                   PACKAGES_DIR,
                   'browser',
                   baseIntegrationFileName.replace('[INTEGRATION_NAME]', integration),
                 ),
-                fileName,
+                path.join(this.localOutPath, fileName),
               );
 
               if (integration === 'feedback') {
-                addStaticAssetSymlink(
-                  this.localOutPath,
+                symlinkAsset(
                   path.resolve(PACKAGES_DIR, 'feedback', 'build/bundles/feedback-modal.js'),
-                  'feedback-modal.bundle.js',
+                  path.join(this.localOutPath, 'feedback-modal.bundle.js'),
                 );
-                addStaticAssetSymlink(
-                  this.localOutPath,
+                symlinkAsset(
                   path.resolve(PACKAGES_DIR, 'feedback', 'build/bundles/feedback-screenshot.js'),
-                  'feedback-screenshot.bundle.js',
+                  path.join(this.localOutPath, 'feedback-screenshot.bundle.js'),
                 );
               }
 
@@ -295,10 +294,9 @@ class SentryScenarioGenerationPlugin {
 
           const baseWasmFileName = BUNDLE_PATHS['wasm']?.[integrationBundleKey];
           if (this.requiresWASMIntegration && baseWasmFileName) {
-            addStaticAssetSymlink(
-              this.localOutPath,
+            symlinkAsset(
               path.resolve(PACKAGES_DIR, 'wasm', baseWasmFileName),
-              'wasm.bundle.js',
+              path.join(this.localOutPath, 'wasm.bundle.js'),
             );
 
             const wasmObject = createHtmlTagObject('script', {

diff --git a/dev-packages/browser-integration-tests/utils/staticAssets.ts b/dev-packages/browser-integration-tests/utils/staticAssets.ts
@@ -22,23 +22,11 @@ export function addStaticAsset(localOutPath: string, fileName: string, cb: () =>
   symlinkAsset(newPath, path.join(localOutPath, fileName));
 }
 
-export function addStaticAssetSymlink(localOutPath: string, originalPath: string, fileName: string): void {
-  const newPath = path.join(STATIC_DIR, fileName);
-
-  // Only copy files once
-  if (!fs.existsSync(newPath)) {
-    fs.symlinkSync(originalPath, newPath);
-  }
-
-  symlinkAsset(newPath, path.join(localOutPath, fileName));
-}
-
-function symlinkAsset(originalPath: string, targetPath: string): void {
+export function symlinkAsset(originalPath: string, targetPath: string): void {
   try {
-    fs.unlinkSync(targetPath);
+    fs.linkSync(originalPath, targetPath);
   } catch {
-    // ignore errors here
+    // ignore errors here, probably means the file already exists
+    // Since we always build into a new directory for each test, we can safely ignore this
   }
-
-  fs.linkSync(originalPath, targetPath);
 }