Add retry in withPage function (#993)

* Add retry in withPage function * Fix lint * enable retry by default * Add env var in tests * Add test to check retry true * Fix tests * Fix tests
percy · Jul 18, 2024 · 57c7d75 · 57c7d75
1 parent 744181b
commit 57c7d75
Show file tree

Hide file tree

Showing 3 changed files with 116 additions and 46 deletions.
diff --git a/src/snapshots.js b/src/snapshots.js
@@ -232,7 +232,7 @@ export async function* takeStorybookSnapshots(percy, callback, { baseUrl, flags
           log.debug(`Page crashed while loading story: ${snapshots[0].name}`);
           // return true to retry as long as the length decreases
           return lastCount > snapshots.length;
-        });
+        }, { snapshotName: snapshots[0].name });
       } catch (e) {
         if (process.env.PERCY_SKIP_STORY_ON_ERROR === 'true') {
           let { name } = snapshots[0];

diff --git a/src/utils.js b/src/utils.js
@@ -1,4 +1,5 @@
 import { request, createRootResource, yieldTo } from '@percy/cli-command/utils';
+import { logger } from '@percy/cli-command';
 import spawn from 'cross-spawn';
 
 // check storybook version
@@ -138,52 +139,66 @@ export function decodeStoryArgs(value) {
 
 // Borrows a percy discovery browser page to navigate to a URL and evaluate a function, returning
 // the results and normalizing any thrown errors.
-export async function* withPage(percy, url, callback, retry) {
-  // provide discovery options that may impact how the page loads
-  let page = yield percy.browser.page({
-    networkIdleTimeout: percy.config.discovery.networkIdleTimeout,
-    requestHeaders: getAuthHeaders(percy.config.discovery),
-    captureMockedServiceWorker: percy.config.discovery.captureMockedServiceWorker,
-    userAgent: percy.config.discovery.userAgent
-  });
+export async function* withPage(percy, url, callback, retry, args) {
+  let log = logger('storybook:utils');
+  let attempt = 0;
+  let retries = 3;
+  while (attempt < retries) {
+    try {
+      // provide discovery options that may impact how the page loads
+      let page = yield percy.browser.page({
+        networkIdleTimeout: percy.config.discovery.networkIdleTimeout,
+        requestHeaders: getAuthHeaders(percy.config.discovery),
+        captureMockedServiceWorker: percy.config.discovery.captureMockedServiceWorker,
+        userAgent: percy.config.discovery.userAgent
+      });
+
+      // patch eval to include storybook specific helpers in the local scope
+      page.eval = (fn, ...args) => page.constructor.prototype.eval.call(page, (
+        typeof fn === 'string' ? fn : [
+          'function withPercyStorybookHelpers() {',
+          `  const VAL_REG = ${VAL_REG};`,
+          `  const NUM_REG = ${NUM_REG};`,
+          `  const HEX_REG = ${HEX_REG};`,
+          `  const COL_REG = ${COL_REG};`,
+          `  const VALID_REG = ${VALID_REG};`,
+          `  return (${fn})(...arguments);`,
+          `  ${isPlainObject}`,
+          `  ${validateStoryArgs}`,
+          `  ${encodeStoryArgs}`,
+          `  ${decodeStoryArgs}`,
+          '}'
+        ].join('\n')
+      ), ...args);
+
+      try {
+        yield page.goto(url);
+        return yield* yieldTo(callback(page));
+      } catch (error) {
+        // if the page crashed and retry returns truthy, try again
+        if (error.message?.includes('crashed') && retry?.()) {
+          return yield* withPage(...arguments);
+        }
 
-  // patch eval to include storybook specific helpers in the local scope
-  page.eval = (fn, ...args) => page.constructor.prototype.eval.call(page, (
-    typeof fn === 'string' ? fn : [
-      'function withPercyStorybookHelpers() {',
-      `  const VAL_REG = ${VAL_REG};`,
-      `  const NUM_REG = ${NUM_REG};`,
-      `  const HEX_REG = ${HEX_REG};`,
-      `  const COL_REG = ${COL_REG};`,
-      `  const VALID_REG = ${VALID_REG};`,
-      `  return (${fn})(...arguments);`,
-      `  ${isPlainObject}`,
-      `  ${validateStoryArgs}`,
-      `  ${encodeStoryArgs}`,
-      `  ${decodeStoryArgs}`,
-      '}'
-    ].join('\n')
-  ), ...args);
-
-  try {
-    yield page.goto(url);
-    return yield* yieldTo(callback(page));
-  } catch (error) {
-    // if the page crashed and retry returns truthy, try again
-    if (error.message?.includes('crashed') && retry?.()) {
-      return yield* withPage(...arguments);
+        /* istanbul ignore next: purposefully not handling real errors */
+        throw (typeof error !== 'string' ? error : new Error(error.replace(
+          // strip generic error names and confusing stack traces
+          /^Error:\s((.+?)\n\s+at\s.+)$/s,
+          // keep the stack trace if the error came from a client script
+          /\n\s+at\s.+?\(https?:/.test(error) ? '$1' : '$2'
+        )));
+      } finally {
+        // always clean up and close the page
+        await page?.close();
+      }
+    } catch (error) {
+      attempt++;
+      let enableRetry = process.env.PERCY_RETRY_STORY_ON_ERROR || 'true';
+      if (!(enableRetry === 'true') || attempt === retries) {
+        throw error;
+      }
+      log.warn(`Retrying Story: ${args.snapshotName}`);
     }
-
-    /* istanbul ignore next: purposefully not handling real errors */
-    throw (typeof error !== 'string' ? error : new Error(error.replace(
-      // strip generic error names and confusing stack traces
-      /^Error:\s((.+?)\n\s+at\s.+)$/s,
-      // keep the stack trace if the error came from a client script
-      /\n\s+at\s.+?\(https?:/.test(error) ? '$1' : '$2'
-    )));
-  } finally {
-    // always clean up and close the page
-    await page?.close();
   }
 }
 
@@ -271,8 +286,8 @@ export function evalSetCurrentStory({ waitFor }, story) {
     let { id, queryParams, globals, args } = story;
 
     // emit a series of events to render the desired story
-    channel.emit('updateGlobals', { globals: {} });
     channel.emit('setCurrentStory', { storyId: id });
+    channel.emit('updateGlobals', { globals: {} });
     channel.emit('updateQueryParams', { ...queryParams });
     if (globals) channel.emit('updateGlobals', { globals: decodeStoryArgs(globals) });
     if (args) channel.emit('updateStoryArgs', { storyId: id, updatedArgs: decodeStoryArgs(args) });

diff --git a/test/storybook.test.js b/test/storybook.test.js
@@ -107,6 +107,7 @@ describe('percy storybook', () => {
   });
 
   it('errors when the client api is missing', async () => {
+    process.env.PERCY_RETRY_STORY_ON_ERROR = false;
     await expectAsync(storybook(['http://localhost:8000'])).toBeRejected();
 
     expect(logger.stderr).toEqual([
@@ -147,6 +148,7 @@ describe('percy storybook', () => {
   });
 
   it('errors when the storybook page errors', async () => {
+    process.env.PERCY_RETRY_STORY_ON_ERROR = false;
     server.reply('/iframe.html', () => [200, 'text/html', [
       `<script>__STORYBOOK_PREVIEW__ = { async extract() { return ${JSON.stringify([
         { id: '1', kind: 'foo', name: 'bar' }
@@ -183,10 +185,12 @@ describe('percy storybook', () => {
   describe('with PERCY_SKIP_STORY_ON_ERROR set to true', () => {
     beforeAll(() => {
       process.env.PERCY_SKIP_STORY_ON_ERROR = true;
+      process.env.PERCY_RETRY_STORY_ON_ERROR = false;
     });
 
     afterAll(() => {
       delete process.env.PERCY_SKIP_STORY_ON_ERROR;
+      delete process.env.PERCY_RETRY_STORY_ON_ERROR;
     });
 
     it('skips the story and logs the error but does not break build', async () => {
@@ -226,6 +230,56 @@ describe('percy storybook', () => {
     });
   });
 
+  describe('with PERCY_RETRY_STORY_ON_ERROR set to true', () => {
+    beforeAll(() => {
+      process.env.PERCY_SKIP_STORY_ON_ERROR = true;
+      process.env.PERCY_RETRY_STORY_ON_ERROR = true;
+    });
+
+    afterAll(() => {
+      delete process.env.PERCY_SKIP_STORY_ON_ERROR;
+      delete process.env.PERCY_RETRY_STORY_ON_ERROR;
+    });
+
+    it('retries story logs the error but does not break build if skip is enabled', async () => {
+      server.reply('/iframe.html', () => [200, 'text/html', [
+        `<script>__STORYBOOK_PREVIEW__ = { async extract() { return ${JSON.stringify([
+          { id: '1', kind: 'foo', name: 'bar' }
+        ])} }, ${
+          'channel: { emit() {}, on: (a, c) => a === "storyErrored" && c(new Error("Story Error")) }'
+        } }</script>`,
+        `<script>__STORYBOOK_STORY_STORE__ = { raw: () => ${JSON.stringify([
+          { id: '1', kind: 'foo', name: 'bar' }
+        ])} }</script>`
+      ].join('')]);
+
+      server.reply('/iframe.html?id=1&viewMode=story', () => [200, 'text/html', [
+        `<script>__STORYBOOK_PREVIEW__ = { async extract() { return ${JSON.stringify([
+          { id: '1', kind: 'foo', name: 'bar' }
+        ])} }, ${
+          'channel: { emit() {}, on: (a, c) => a === "storyErrored" && c(new Error("Story Error")) }'
+        } }</script>`,
+        `<script>__STORYBOOK_STORY_STORE__ = { raw: () => ${JSON.stringify([
+          { id: '1', kind: 'foo', name: 'bar' }
+        ])} }</script>`
+      ].join('')]);
+
+      // does not reject
+      await storybook(['http://localhost:8000']);
+
+      // contains logs of story error
+      expect(logger.stderr).toEqual([
+        '[percy] Retrying Story: foo: bar',
+        '[percy] Retrying Story: foo: bar',
+        '[percy] Failed to capture story: foo: bar',
+        // error logs contain the client stack trace
+        jasmine.stringMatching(/^\[percy\] Error: Story Error\n.*\/iframe\.html.*$/s),
+        // does not create a build if all stories failed [ 1 in this case ]
+        '[percy] Build not created'
+      ]);
+    });
+  });
+
   it('uses the preview dom when javascript is enabled', async () => {
     const FAKE_PREVIEW_V8 = `{ async extract() { return ${JSON.stringify([
       { id: '1', kind: 'foo', name: 'bar' },
@@ -514,6 +568,7 @@ describe('percy storybook', () => {
   });
 
   it('handles page crashes while taking snapshots', async () => {
+    process.env.PERCY_RETRY_STORY_ON_ERROR = false;
     // eslint-disable-next-line import/no-extraneous-dependencies
     let { Percy } = await import('@percy/core');