✨ Analyze Error Logs and Suggest Fixes for Issues (#1622)

* feat: initial commit for sending suggestions for the fix * feat: api changes for showing suggestions * feat: added suggestions for browser lauch failure + discovery errors + snapshot errors * chore: remove async from network.js * feat: refactored some code + lint fix + added some comments at some places * feat: refactored client.js and percy.js * test: added tests for percy client formatAndSendErrorLog * test: added tests for percy client coverage fix * test: added tests for percy client coverage fix branch coverage * test: added tests for percy core discovery.js * test: added tests for percy core percy.js * test: added tests for percy core percy.js coverage fix * fix: moved percy.start to try catch in cli-exec test: added test cases for the same * feat: comment fix + throw error if comparison tile upload verify is failed feat: correct some error message + added suggestion message for proxy issues test: added some more tests cases for reliability * test: added test fix for exec.test.js + start.test.js + upload.test.js * test: coverage fix * chore: lint fix * feat: added suggestions in case of proxy issues * test: coverage fix for proxy.js * refactor: minor refactoring in percy.js/proxy.js + some comment fix
percy · Jun 27, 2024 · 54ef405 · 54ef405
1 parent 1b93761
commit 54ef405
Show file tree

Hide file tree

Showing 14 changed files with 624 additions and 36 deletions.
diff --git a/packages/cli-exec/src/start.js b/packages/cli-exec/src/start.js
@@ -19,10 +19,10 @@ export const start = command('start', {
     log.debug('Skipping percy project attribute calculation');
   }
 
-  // start percy
-  yield* percy.yield.start();
-
   try {
+    // start percy
+    yield* percy.yield.start();
+
     // run until stopped or terminated
     yield* yieldFor(() => percy.readyState >= 3);
   } catch (error) {

diff --git a/packages/cli-exec/test/exec.test.js b/packages/cli-exec/test/exec.test.js
@@ -75,7 +75,15 @@ describe('percy exec', () => {
   it('starts and stops the percy process around the command', async () => {
     await exec(['--', 'node', '--eval', '']);
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual(jasmine.arrayContaining([
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/'
+    ]));
+
     expect(logger.stdout).toEqual([
       '[percy] Percy has started!',
       '[percy] Running "node --eval "',
@@ -126,7 +134,14 @@ describe('percy exec', () => {
       exec(['--', 'node', '--eval', 'process.exit(3)'])
     ).toBeRejectedWithError('EEXIT: 3');
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual(jasmine.arrayContaining([
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/'
+    ]));
     expect(logger.stdout).toEqual(jasmine.arrayContaining([
       '[percy] Percy has started!',
       '[percy] Running "node --eval process.exit(3)"',
@@ -139,7 +154,14 @@ describe('percy exec', () => {
     await exec(['--', 'echo', 'Hi!']);
 
     expect(stdoutSpy).toHaveBeenCalled();
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual(jasmine.arrayContaining([
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/'
+    ]));
     expect(logger.stdout).toEqual(jasmine.arrayContaining([
       '[percy] Percy has started!',
       '[percy] Running "echo Hi!"',
@@ -154,9 +176,16 @@ describe('percy exec', () => {
       exec(['--', 'node', './test/test-data/test_prog.js', 'error']) // Throws Error
     ).toBeRejectedWithError('EEXIT: 1');
 
-    expect(logger.stderr).toEqual([
+    expect(logger.stderr).toEqual(jasmine.arrayContaining([
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/',
       '[percy] Notice: Percy collects CI logs for service improvement, stored for 30 days. Opt-out anytime with export PERCY_CLIENT_ERROR_LOGS=false'
-    ]);
+    ]));
+
     expect(logger.stdout).toEqual(jasmine.arrayContaining([
       '[percy] Percy has started!',
       '[percy] Running "node ./test/test-data/test_prog.js error"',
@@ -216,9 +245,15 @@ describe('percy exec', () => {
 
     await expectAsync(exec(['--', 'foobar'])).toBeRejected();
     expect(stdinSpy).toHaveBeenCalled();
-    expect(logger.stderr).toEqual([
-      '[percy] Error: spawn error'
-    ]);
+    console.log(logger.stderr);
+    expect(logger.stderr).toEqual(jasmine.arrayContaining([
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/'
+    ]));
     expect(logger.stdout).toEqual(jasmine.arrayContaining([
       '[percy] Percy has started!',
       '[percy] Running "foobar"',
@@ -244,7 +279,15 @@ describe('percy exec', () => {
     // user termination is not considered an error
     await expectAsync(test).toBeResolved();
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual(jasmine.arrayContaining([
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/'
+    ]));
+
     expect(logger.stdout).toContain(
       '[percy] Stopping percy...'
     );
@@ -259,7 +302,15 @@ describe('percy exec', () => {
       'await request(url).catch(e => (console.error(e), process.exit(2)));'
     ].join('')]);
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual(jasmine.arrayContaining([
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/'
+    ]));
+
     expect(logger.stdout).toEqual(jasmine.arrayContaining([
       '[percy] Percy has started!',
       jasmine.stringMatching('\\[percy] Running "node '),
@@ -272,7 +323,14 @@ describe('percy exec', () => {
       'process.env.PERCY_BUILD_ID === "123" || process.exit(2)'
     )]);
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual(jasmine.arrayContaining([
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/'
+    ]));
     expect(logger.stdout).toEqual(jasmine.arrayContaining([
       '[percy] Percy has started!',
       jasmine.stringMatching('\\[percy] Running "node '),
@@ -285,7 +343,14 @@ describe('percy exec', () => {
       'process.env.PERCY_BUILD_URL === "https://percy.io/test/test/123" || process.exit(2)'
     )]);
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual(jasmine.arrayContaining([
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/'
+    ]));
     expect(logger.stdout).toEqual(jasmine.arrayContaining([
       '[percy] Percy has started!',
       jasmine.stringMatching('\\[percy] Running "node '),

diff --git a/packages/cli-exec/test/start.test.js b/packages/cli-exec/test/start.test.js
@@ -1,5 +1,5 @@
 import { request } from '@percy/cli-command/utils';
-import { logger, setupTest } from '@percy/cli-command/test/helpers';
+import { logger, setupTest, api } from '@percy/cli-command/test/helpers';
 import { start, ping } from '@percy/cli-exec';
 
 describe('percy exec:start', () => {
@@ -91,9 +91,24 @@ describe('percy exec:start', () => {
 
     await expectAsync(start()).toBeRejected();
 
-    expect(logger.stdout).toEqual([]);
+    expect(logger.stdout).toEqual(jasmine.arrayContaining([
+      "[percy] Build's CLI and CI logs sent successfully. Please share this log ID with Percy team in case of any issues - random_sha"
+    ]));
+
+    let lastReq = api.requests['/suggestions/from_logs'].length - 1;
+
+    expect(api.requests['/suggestions/from_logs'][lastReq].body).toEqual({
+      data: {
+        logs: [
+          { message: 'Percy is already running or the port 5338 is in use' }
+        ]
+      }
+    });
+
     expect(logger.stderr).toEqual(jasmine.arrayContaining([
-      '[percy] Error: Percy is already running or the port is in use'
+      '[percy] Notice: Percy collects CI logs for service improvement, stored for 30 days. Opt-out anytime with export PERCY_CLIENT_ERROR_LOGS=false',
+      '[percy] Error: Percy is already running or the port 5338 is in use',
+      '[percy] Error: Percy is already running or the port 5338 is in use'
     ]));
   });
 

diff --git a/packages/cli-upload/test/upload.test.js b/packages/cli-upload/test/upload.test.js
@@ -199,8 +199,15 @@ describe('percy upload', () => {
     await up;
 
     expect(logger.stderr).toEqual([
-      '[percy] AbortError: SIGTERM'
+      '[percy] AbortError: SIGTERM',
+      '[percy] Detected error for percy build',
+      '[percy] Failure: Snapshot command was not called',
+      '[percy] Failure Reason: Snapshot Command was not called. please check your CI for errors',
+      '[percy] Suggestion: Try using percy snapshot command to take snapshots',
+      '[percy] Refer to the below Doc Links for the same',
+      '[percy] * https://www.browserstack.com/docs/percy/take-percy-snapshots/'
     ]);
+
     expect(logger.stdout).toEqual(jasmine.arrayContaining([
       '[percy] Percy has started!',
       '[percy] Uploading 3 snapshots...',

diff --git a/packages/client/src/client.js b/packages/client/src/client.js
@@ -13,6 +13,7 @@ import {
   getPackageJSON,
   waitForTimeout,
   validateTiles,
+  formatLogErrors,
   tagsList
 } from './utils.js';
 
@@ -544,8 +545,12 @@ export class PercyClient {
     while (retries > 0 && !success);
 
     if (!success) {
-      this.log.error('Uploading comparison tile failed');
-      return false;
+      let errMsg = 'Uploading comparison tile failed';
+
+      // Detecting error and logging fix for the same
+      // We are throwing this error as the comparison will be failed
+      // even if 1 tile gets failed
+      throw new Error(errMsg);
     }
     return true;
   }
@@ -617,6 +622,15 @@ export class PercyClient {
     });
   }
 
+  async getErrorAnalysis(errors) {
+    const errorLogs = formatLogErrors(errors);
+    this.log.debug('Sending error logs for analysis');
+
+    return this.post('suggestions/from_logs', {
+      data: errorLogs
+    });
+  }
+
   mayBeLogUploadSize(contentSize) {
     if (contentSize >= 25 * 1024 * 1024) {
       this.log.error('Uploading resource above 25MB might fail the build...');

diff --git a/packages/client/src/proxy.js b/packages/client/src/proxy.js
@@ -169,6 +169,14 @@ export class ProxyHttpsAgent extends https.Agent {
     let handleError = err => {
       socket.destroy(err);
       logger('client:proxy').error(`Proxying request failed: ${err}`);
+
+      // We don't get statusCode here, relying on checking error message only
+      if (!!err.message && (err.message?.includes('ECONNREFUSED') || err.message?.includes('EHOSTUNREACH'))) {
+        logger('client:proxy').warn('If needed, Please verify if your proxy credentials are correct');
+        logger('client:proxy').warn('Please check if your proxy is set correctly and reachable');
+      }
+
+      logger('client:proxy').warn('Please check network connection, proxy and ensure that following domains are whitelisted: github.com, percy.io, storage.googleapis.com. In case you are an enterprise customer make sure to whitelist "percy-enterprise.browserstack.com" as well.');
       callback(err);
     };
 

diff --git a/packages/client/src/utils.js b/packages/client/src/utils.js
@@ -213,6 +213,26 @@ export function validateTiles(tiles) {
   return true;
 }
 
+export function formatLogErrors(errorLogs) {
+  let errors = [];
+  if (typeof errorLogs === 'string') {
+    errors.push({
+      message: errorLogs
+    });
+  } else if (Array.isArray(errorLogs)) {
+    errors = errorLogs;
+  } else {
+    errors.push({
+      message: errorLogs
+    });
+    errors.push({
+      message: errorLogs?.message || ''
+    });
+  }
+
+  return { logs: errors };
+}
+
 // convert tags comma-separated-names to array of objects for POST request
 export function tagsList(tags) {
   let tagsArr = [];
@@ -226,6 +246,7 @@ export function tagsList(tags) {
 
 export {
   hostnameMatches,
+  getProxy,
   ProxyHttpAgent,
   ProxyHttpsAgent,
   proxyAgentFor

diff --git a/packages/client/test/client.test.js b/packages/client/test/client.test.js
@@ -1303,16 +1303,14 @@ describe('PercyClient', () => {
       ]);
     });
 
-    it('returns false if tile is not verified', async () => {
+    it('throws error if tile is not verified', async () => {
       api.reply('/comparisons/891011/tiles/verify', async () => {
         return [400, 'failure'];
       });
 
       await expectAsync(client.uploadComparisonTiles(891011, [
         { sha: sha256hash('foo') }
-      ])).toBeResolvedTo([
-        false
-      ]);
+      ])).toBeRejectedWithError('Uploading comparison tile failed');
     });
 
     it('throws any errors from verifying', async () => {
@@ -1348,6 +1346,10 @@ describe('PercyClient', () => {
     });
 
     it('verify a comparison tile', async () => {
+      api.reply('/comparisons/123/tiles/verify', async () => {
+        return [200, 'success'];
+      });
+
       await expectAsync(client.verify(123, 'sha')).toBeResolved();
 
       expect(api.requests['/comparisons/123/tiles/verify']).toBeDefined();
@@ -1572,6 +1574,44 @@ describe('PercyClient', () => {
     });
   });
 
+  describe('#getErrorAnalysis', () => {
+    const sharedTest = (reqBody, expectedBody) => {
+      it('should send error logs to API', async () => {
+        await expectAsync(client.getErrorAnalysis(reqBody)).toBeResolved();
+
+        expect(api.requests['/suggestions/from_logs']).toBeDefined();
+        expect(api.requests['/suggestions/from_logs'][0].method).toBe('POST');
+        expect(api.requests['/suggestions/from_logs'][0].body).toEqual({
+          data: {
+            logs: expectedBody
+          }
+        });
+      });
+    };
+    describe('when error is of type array', () => {
+      let body = [
+        { message: 'Some Error Log' },
+        { message: 'Some Error logs 2' }
+      ];
+
+      // Here requestedBody and expectedBody should be same
+      sharedTest(body, body);
+    });
+
+    describe('when error is of type string', () => {
+      sharedTest('some error', [{ message: 'some error' }]);
+    });
+
+    describe('when error is of type object', () => {
+      sharedTest({
+        some_key: 'some_error'
+      }, [
+        { message: { some_key: 'some_error' } },
+        { message: '' }
+      ]);
+    });
+  });
+
   describe('#mayBeLogUploadSize', () => {
     it('does not warns when upload size less 20MB/25MB', () => {
       client.mayBeLogUploadSize(1000);