change structure of artifacts zip

Author: jochen-testingbot

src/providers/maestro.ts

@@ -2479,21 +2479,30 @@ export default class Maestro extends BaseProvider<MaestroOptions> {
     return s;
   }
 
-  private buildFlowDirNames(flows: MaestroFlowInfo[]): Map<number, string> {
-    const baseNames = new Map<number, string>();
+  private buildFlowDirNames(
+    entries: Array<{ runId: number; flow: MaestroFlowInfo }>,
+    reserved: Set<string> = new Set(),
+  ): Map<string, string> {
+    const baseNames = new Map<string, string>();
     const counts = new Map<string, number>();
 
-    for (const flow of flows) {
+    for (const r of reserved) {
+      counts.set(r, 1);
+    }
+
+    for (const { runId, flow } of entries) {
       const sanitized = this.sanitizeFlowDirName(flow.name);
-      const base = sanitized ? `flow_${sanitized}` : `flow_${flow.id}`;
-      baseNames.set(flow.id, base);
+      const base = sanitized || `flow_${flow.id}`;
+      baseNames.set(`${runId}:${flow.id}`, base);
       counts.set(base, (counts.get(base) || 0) + 1);
     }
 
-    const result = new Map<number, string>();
-    for (const flow of flows) {
-      const base = baseNames.get(flow.id)!;
-      result.set(flow.id, (counts.get(base) || 0) > 1 ? `${base}_${flow.id}` : base);
+    const result = new Map<string, string>();
+    for (const { runId, flow } of entries) {
+      const key = `${runId}:${flow.id}`;
+      const base = baseNames.get(key)!;
+      const collides = (counts.get(base) || 0) > 1;
+      result.set(key, collides ? `${base}_${runId}_${flow.id}` : base);
     }
     return result;
   }
@@ -2601,84 +2610,110 @@ export default class Maestro extends BaseProvider<MaestroOptions> {
     );
 
     try {
+      const runDetailsList: Array<{
+        run: MaestroRunInfo;
+        details: MaestroRunDetails;
+      }> = [];
+
       for (const run of runsToDownload) {
+        if (!this.options.quiet) {
+          logger.info(`  Waiting for artifacts sync for run ${run.id}...`);
+        }
         try {
-          if (!this.options.quiet) {
-            logger.info(`  Waiting for artifacts sync for run ${run.id}...`);
-          }
+          const details = await this.waitForArtifactsSync(run.id);
+          runDetailsList.push({ run, details });
+        } catch (error) {
+          logger.error(
+            `Failed to download artifacts for run ${run.id}: ${error instanceof Error ? error.message : error}`,
+          );
+        }
+      }
 
-          const runDetails = await this.waitForArtifactsSync(run.id);
+      const multiRun = runDetailsList.length > 1;
+      const runReportName = (runId: number): string =>
+        multiRun ? `report_${runId}.xml` : 'report.xml';
+      const runAssetsDirName = (runId: number): string =>
+        multiRun ? `run_${runId}` : '';
 
-          const flowsWithAssets = (runDetails.flows || []).filter(
-            (flow) => flow.assets,
-          );
+      const reservedNames = new Set<string>();
+      for (const { run, details } of runDetailsList) {
+        if (details.report) reservedNames.add(runReportName(run.id));
+        if (details.assets) {
+          const dir = runAssetsDirName(run.id);
+          if (dir) reservedNames.add(dir);
+        }
+      }
 
-          if (!runDetails.assets && flowsWithAssets.length === 0) {
-            if (!this.options.quiet) {
-              logger.info(`  No artifacts available for run ${run.id}`);
-            }
-            continue;
-          }
+      const flowEntries = runDetailsList.flatMap(({ run, details }) =>
+        (details.flows || [])
+          .filter((flow) => flow.assets)
+          .map((flow) => ({ runId: run.id, flow })),
+      );
+      const flowDirNames = this.buildFlowDirNames(flowEntries, reservedNames);
 
-          const runDir = path.join(tempDir, `run_${run.id}`);
-          await fs.promises.mkdir(runDir, { recursive: true });
+      for (const { run, details } of runDetailsList) {
+        const flowsWithAssets = (details.flows || []).filter(
+          (flow) => flow.assets,
+        );
 
-          if (runDetails.assets) {
-            await this.downloadAssetBundle(runDetails.assets, runDir);
+        if (!details.assets && flowsWithAssets.length === 0) {
+          if (!this.options.quiet) {
+            logger.info(`  No artifacts available for run ${run.id}`);
           }
+          continue;
+        }
 
-          const flowDirNames = this.buildFlowDirNames(flowsWithAssets);
-
-          for (const flow of flowsWithAssets) {
-            const flowDirName = flowDirNames.get(flow.id)!;
-            const flowDir = path.join(runDir, flowDirName);
-            await fs.promises.mkdir(flowDir, { recursive: true });
-            await this.downloadAssetBundle(flow.assets!, flowDir);
-
-            if (flow.report) {
-              const flowReportPath = path.join(flowDir, 'report.xml');
-              try {
-                await fs.promises.writeFile(
-                  flowReportPath,
-                  flow.report,
-                  'utf-8',
-                );
-                if (!this.options.quiet) {
-                  logger.info(`    Saved ${flowDirName} report.xml`);
-                }
-              } catch (error) {
-                logger.error(
-                  `    Failed to save report.xml for ${flowDirName}: ${error instanceof Error ? error.message : error}`,
-                );
-              }
-            }
+        if (details.assets) {
+          const dirName = runAssetsDirName(run.id);
+          const targetDir = dirName ? path.join(tempDir, dirName) : tempDir;
+          if (dirName) {
+            await fs.promises.mkdir(targetDir, { recursive: true });
           }
+          await this.downloadAssetBundle(details.assets, targetDir);
+        }
+
+        for (const flow of flowsWithAssets) {
+          const flowDirName = flowDirNames.get(`${run.id}:${flow.id}`)!;
+          const flowDir = path.join(tempDir, flowDirName);
+          await fs.promises.mkdir(flowDir, { recursive: true });
+          await this.downloadAssetBundle(flow.assets!, flowDir);
 
-          if (runDetails.report) {
-            const reportPath = path.join(runDir, 'report.xml');
+          if (flow.report) {
+            const flowReportPath = path.join(flowDir, 'report.xml');
             try {
               await fs.promises.writeFile(
-                reportPath,
-                runDetails.report,
+                flowReportPath,
+                flow.report,
                 'utf-8',
               );
               if (!this.options.quiet) {
-                logger.info(`    Saved report.xml`);
+                logger.info(`    Saved ${flowDirName}/report.xml`);
               }
             } catch (error) {
               logger.error(
-                `    Failed to save report.xml: ${error instanceof Error ? error.message : error}`,
+                `    Failed to save report.xml for ${flowDirName}: ${error instanceof Error ? error.message : error}`,
               );
             }
           }
+        }
 
-          if (!this.options.quiet) {
-            logger.info(`  Artifacts for run ${run.id} downloaded`);
+        if (details.report) {
+          const reportName = runReportName(run.id);
+          const reportPath = path.join(tempDir, reportName);
+          try {
+            await fs.promises.writeFile(reportPath, details.report, 'utf-8');
+            if (!this.options.quiet) {
+              logger.info(`    Saved ${reportName}`);
+            }
+          } catch (error) {
+            logger.error(
+              `    Failed to save ${reportName}: ${error instanceof Error ? error.message : error}`,
+            );
           }
-        } catch (error) {
-          logger.error(
-            `Failed to download artifacts for run ${run.id}: ${error instanceof Error ? error.message : error}`,
-          );
+        }
+
+        if (!this.options.quiet) {
+          logger.info(`  Artifacts for run ${run.id} downloaded`);
         }
       }
 

tests/providers/maestro.test.ts

@@ -2397,13 +2397,9 @@ describe('Maestro', () => {
         },
       ]);
 
-      const runDir = path.join(tempDir, 'run_5678');
-      const flow11Dir = path.join(runDir, 'flow_login_flow.yaml');
-      const flow12Dir = path.join(runDir, 'flow_checkout.yaml');
+      const flow11Dir = path.join(tempDir, 'login_flow.yaml');
+      const flow12Dir = path.join(tempDir, 'checkout.yaml');
 
-      expect(fs.promises.mkdir).toHaveBeenCalledWith(runDir, {
-        recursive: true,
-      });
       expect(fs.promises.mkdir).toHaveBeenCalledWith(flow11Dir, {
         recursive: true,
       });
@@ -2434,11 +2430,17 @@ describe('Maestro', () => {
         'utf-8',
       );
       expect(fs.promises.writeFile).toHaveBeenCalledWith(
-        path.join(runDir, 'report.xml'),
+        path.join(tempDir, 'report.xml'),
         '<run-report/>',
         'utf-8',
       );
 
+      // Single run: no run-level wrapper directory should be created.
+      const mkdirPaths = (fs.promises.mkdir as jest.Mock).mock.calls.map(
+        (call) => call[0],
+      );
+      expect(mkdirPaths).not.toContain(path.join(tempDir, 'run_5678'));
+
       const noArtifactsLog = consoleSpy.mock.calls.find(
         (call) =>
           typeof call[0] === 'string' &&
@@ -2451,38 +2453,52 @@ describe('Maestro', () => {
 
     it('should sanitize, cap, disambiguate, and fall back when building flow dir names', () => {
       const longName = 'a'.repeat(100);
-      const flows: MaestroFlowInfo[] = [
-        { id: 1, name: 'Login Flow', status: 'DONE' },
-        { id: 2, name: 'login flow', status: 'DONE' },
-        { id: 3, name: 'Login Flow', status: 'DONE' },
-        { id: 4, name: '', status: 'DONE' },
-        { id: 5, name: '!!! @@@ ###', status: 'DONE' },
-        { id: 6, name: longName, status: 'DONE' },
+      const entries = [
+        { runId: 100, flow: { id: 1, name: 'Login Flow', status: 'DONE' as MaestroFlowStatus } },
+        { runId: 100, flow: { id: 2, name: 'login flow', status: 'DONE' as MaestroFlowStatus } },
+        { runId: 100, flow: { id: 3, name: 'Login Flow', status: 'DONE' as MaestroFlowStatus } },
+        { runId: 100, flow: { id: 4, name: '', status: 'DONE' as MaestroFlowStatus } },
+        { runId: 100, flow: { id: 5, name: '!!! @@@ ###', status: 'DONE' as MaestroFlowStatus } },
+        { runId: 100, flow: { id: 6, name: longName, status: 'DONE' as MaestroFlowStatus } },
       ];
 
-      const names = maestro['buildFlowDirNames'](flows);
+      const names = maestro['buildFlowDirNames'](entries);
 
       // No collisions
       const values = Array.from(names.values());
       expect(new Set(values).size).toBe(values.length);
 
-      // Distinct sanitized name kept as-is
-      expect(names.get(2)).toBe('flow_login_flow');
+      // Distinct sanitized name kept as-is (no flow_ prefix)
+      expect(names.get('100:2')).toBe('login_flow');
 
-      // Collisions get _<id> suffix
-      expect(names.get(1)).toBe('flow_Login_Flow_1');
-      expect(names.get(3)).toBe('flow_Login_Flow_3');
+      // Collisions get _<runId>_<flowId> suffix
+      expect(names.get('100:1')).toBe('Login_Flow_100_1');
+      expect(names.get('100:3')).toBe('Login_Flow_100_3');
 
       // Empty / unsanitizable names fall back to flow_<id>
-      expect(names.get(4)).toBe('flow_4');
-      expect(names.get(5)).toBe('flow_5');
+      expect(names.get('100:4')).toBe('flow_4');
+      expect(names.get('100:5')).toBe('flow_5');
 
-      // Long names are capped (sanitized portion <= 64 chars)
-      const dirName = names.get(6)!;
-      const sanitized = dirName.replace(/^flow_/, '').replace(/_\d+$/, '');
+      // Long names are capped at 64 chars (or 64 + suffix when colliding)
+      const dirName = names.get('100:6')!;
+      const sanitized = dirName.replace(/_\d+_\d+$/, '');
       expect(sanitized.length).toBeLessThanOrEqual(64);
     });
 
+    it('should disambiguate flows that would collide with reserved names', () => {
+      const entries = [
+        { runId: 100, flow: { id: 1, name: 'report.xml', status: 'DONE' as MaestroFlowStatus } },
+      ];
+      const reserved = new Set(['report.xml']);
+
+      const names = maestro['buildFlowDirNames'](entries, reserved);
+
+      // The flow's base would be "report.xml" but that name is taken
+      // by the run-level report file → must be disambiguated.
+      expect(names.get('100:1')).not.toBe('report.xml');
+      expect(names.get('100:1')).toBe('report.xml_100_1');
+    });
+
     it('should log "No artifacts available" when neither run nor flows have assets', async () => {
       const optionsWithArtifacts = new MaestroOptions(
         'path/to/app.apk',