core(asset-saver): fix handling of undefined trace (GoogleChrome#15451)

olamidepeterojo · Sep 14, 2023 · 9152513 · 9152513
1 parent d97fc99
commit 9152513
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 13 deletions.
diff --git a/core/lib/asset-saver.js b/core/lib/asset-saver.js
@@ -29,8 +29,8 @@ const stepDirectoryRegex = /^step(\d+)$/;
 
 /**
  * @typedef {object} PreparedAssets
- * @property {LH.Trace} traceData
- * @property {LH.DevtoolsLog} devtoolsLog
+ * @property {LH.Trace} [traceData]
+ * @property {LH.DevtoolsLog} [devtoolsLog]
  */
 
 
@@ -420,14 +420,18 @@ async function saveLanternDebugTraces(pathWithBasename) {
  */
 async function saveAssets(artifacts, audits, pathWithBasename) {
   const allAssets = await prepareAssets(artifacts, audits);
-  const saveAll = allAssets.map(async (passAssets, index) => {
-    const devtoolsLogFilename = `${pathWithBasename}-${index}${devtoolsLogSuffix}`;
-    fs.writeFileSync(devtoolsLogFilename, JSON.stringify(passAssets.devtoolsLog, null, 2));
-    log.log('saveAssets', 'devtools log saved to disk: ' + devtoolsLogFilename);
-
-    const traceFilename = `${pathWithBasename}-${index}${traceSuffix}`;
-    await saveTrace(passAssets.traceData, traceFilename);
-    log.log('saveAssets', 'trace file streamed to disk: ' + traceFilename);
+  const saveAll = allAssets.map(async (assets, index) => {
+    if (assets.devtoolsLog) {
+      const devtoolsLogFilename = `${pathWithBasename}-${index}${devtoolsLogSuffix}`;
+      await saveDevtoolsLog(assets.devtoolsLog, devtoolsLogFilename);
+      log.log('saveAssets', 'devtools log saved to disk: ' + devtoolsLogFilename);
+    }
+
+    if (assets.traceData) {
+      const traceFilename = `${pathWithBasename}-${index}${traceSuffix}`;
+      await saveTrace(assets.traceData, traceFilename);
+      log.log('saveAssets', 'trace file streamed to disk: ' + traceFilename);
+    }
   });
 
   await Promise.all(saveAll);

diff --git a/core/test/lib/asset-saver-test.js b/core/test/lib/asset-saver-test.js
@@ -33,14 +33,14 @@ describe('asset-saver helper', () => {
   describe('saves files', function() {
     const tmpDir = `${LH_ROOT}/.tmp/asset-saver-test`;
 
-    before(() => {
+    before(async () => {
       fs.mkdirSync(tmpDir, {recursive: true});
       const artifacts = {
         DevtoolsLog: [{message: 'first'}, {message: 'second'}],
         Trace: {traceEvents},
       };
 
-      return assetSaver.saveAssets(artifacts, dbwResults.audits, `${tmpDir}/the_file`);
+      await assetSaver.saveAssets(artifacts, dbwResults.audits, `${tmpDir}/the_file`);
     });
 
     it('trace file saved to disk with trace events and extra fakeEvents', () => {
@@ -57,9 +57,35 @@ describe('asset-saver helper', () => {
     it('devtools log file saved to disk with data', () => {
       const filename = tmpDir + '/the_file-0.devtoolslog.json';
       const fileContents = fs.readFileSync(filename, 'utf8');
-      assert.ok(fileContents.includes('"message": "first"'));
+      assert.ok(fileContents.includes('"message":"first"'));
       fs.unlinkSync(filename);
     });
+
+    it('skips trace when undefined', async () => {
+      const noTracePrefix = tmpDir + '/the_file_no_trace';
+      const artifacts = {
+        DevtoolsLog: [{message: 'first'}, {message: 'second'}],
+      };
+      await assetSaver.saveAssets(artifacts, dbwResults.audits, noTracePrefix);
+
+      assert.ok(fs.existsSync(noTracePrefix + '-0.devtoolslog.json'));
+      fs.unlinkSync(noTracePrefix + '-0.devtoolslog.json');
+
+      assert.ok(!fs.existsSync(noTracePrefix + '-0.trace.json'));
+    });
+
+    it('skips dt log when undefined', async () => {
+      const noDtLogPrefix = tmpDir + '/the_file_no_dt';
+      const artifacts = {
+        Trace: {traceEvents},
+      };
+      await assetSaver.saveAssets(artifacts, dbwResults.audits, noDtLogPrefix);
+
+      assert.ok(!fs.existsSync(noDtLogPrefix + '-0.devtoolslog.json'));
+
+      assert.ok(fs.existsSync(noDtLogPrefix + '-0.trace.json'));
+      fs.unlinkSync(noDtLogPrefix + '-0.trace.json');
+    });
   });
 
   describe('prepareAssets', () => {