Misc bug fixing

app/budgets.js

@@ -253,7 +253,7 @@ function processLightWallet(response, budgets, handleFailures) {
     return Object.values(resourceOutputs).reduce((result, { header, output }) => {
       if (result && output) {
         result += `\n${header}${output}`;
-      } else {
+      } else if (output) {
         result += `${header}${output}`;
       }
       return result;
@@ -268,7 +268,7 @@ function processBudgets(urlRoute, stats, budgets = []) {
     const output = runBudget(stats);
     if (reportOutput && output) {
       reportOutput += `\n${output}`;
-    } else {
+    } else if (output) {
       reportOutput += output;
     }
   });
@@ -279,7 +279,7 @@ function processBudgets(urlRoute, stats, budgets = []) {
     const output = Object.values(outputs).reduce((accOutput, { output: budgetOutput }) => {
       if (accOutput && budgetOutput) {
         accOutput += `\n${budgetOutput}`;
-      } else {
+      } else if (budgetOutput) {
         accOutput += budgetOutput;
       }
       return accOutput;

app/configuration.js

@@ -1,13 +1,11 @@
-const { homepage } = require('../package.json');
-
 const { CONFIG_FILE_PATH = '.github/lightkeeper.json' } = process.env;
 
 class Configuration {
-  constructor(params, status) {
+  constructor(params, status, detailsUrl) {
     this.params = params;
     this.status = status;
-    this.detailsUrl = `${homepage}/#configuration`;
-    this.requiredKeys = ['baseUrl', 'ci'];
+    this.detailsUrl = detailsUrl;
+    this.requiredKeys = ['baseUrl', 'ci', 'type'];
   }
 
   /**
@@ -17,6 +15,7 @@ class Configuration {
     const { context, github, headBranch: ref, pullNumber: pull_number } = this.params;
     const { owner, repo } = context.repo();
     const { data: prFiles } = await github.pullRequests.listFiles(context.repo({ pull_number }));
+    // TODO: if removed, throw 404 status
     const modifiedFiles = prFiles
       .filter(file => ['modified', 'added'].includes(file.status))
       .map(file => file.filename);
@@ -43,6 +42,7 @@ class Configuration {
   async getConfiguration() {
     let configuration = {};
     let missingKeys = this.requiredKeys;
+    let parseError = false;
     try {
       const {
         data: { content }
@@ -56,19 +56,27 @@ class Configuration {
       if (error.status === 404) {
         return configuration;
       }
+      parseError = error.message;
     }
     // Check for required keys
     if (configuration) {
       missingKeys = missingKeys.filter(
         key => !(configuration[key] && typeof configuration[key] === 'string')
       );
       if (missingKeys.length) {
+        let outputTitle = `Missing required keys or invalid types: ${missingKeys.join(', ')}`;
+        if (parseError) {
+          outputTitle = parseError;
+        }
+        const { output: { title = '' } = {} } = await this.status.find();
+        if (title === outputTitle) {
+          return {};
+        }
         this.status.run({
           conclusion: 'action_required',
-          details_url: this.detailsUrl,
           output: {
-            title: `Missing required keys or invalid types: ${missingKeys.join(',')}`,
-            summary: `More info at: ${this.detailsUrl}`
+            title: outputTitle,
+            summary: this.detailsUrl
           }
         });
         return {};

app/index.js

@@ -50,7 +50,7 @@ async function onCompletedCheck(context) {
     context,
     null,
     { pullNumber, headBranch, headSha, installationNode },
-    isValidCheck([name, checkAppName, login])
+    isValidCheck([name, checkAppName, login], 'check')
   );
 }
 

app/report.js

@@ -2,6 +2,7 @@ const { detailsSummary } = require('./util');
 
 function prepareReport(order, reports) {
   let reportSummary = '';
+  let warningsFound = false;
   const commentStats = {
     '⬆️': {
       body: '',
@@ -25,6 +26,9 @@ function prepareReport(order, reports) {
       if (!comment || !output) return;
       comment.count += 1;
       comment.body += output;
+      if (icon === '⚠️') {
+        warningsFound = true;
+      }
     });
   });
 
@@ -45,16 +49,17 @@ function prepareReport(order, reports) {
     commentSummary = `# 🚢 Lightkeeper Report\n${commentSummary}`;
   }
 
-  const getTitle = (conclusion, errors) => {
+  const getTitle = (conclusion, errors, warnings) => {
     let title = '';
     const urlText = `${order.length} URL${order.length > 1 ? 's' : ''}`;
     const errorsFound = `${errors} error${errors > 1 ? 's' : ''}`;
+
     switch (conclusion) {
       case 'failure':
         title = `Found ${errorsFound} across ${urlText}.`;
         break;
       case 'neutral':
-        title = 'Non-critical errors were found.';
+        title = warnings && !errors ? '⚠️ Passed with warnings.' : '⚠️ Non-critical errors found.';
         break;
       default:
         title = 'All tests passed! See the full report. ➡️';
@@ -65,7 +70,8 @@ function prepareReport(order, reports) {
   return {
     reportSummary,
     commentSummary,
-    getTitle
+    getTitle,
+    warningsFound
   };
 }
 

app/runner.js

@@ -1,4 +1,5 @@
 const axios = require('axios');
+const merge = require('lodash.merge');
 const { parseConfig } = require('./util');
 
 const { LIGHTHOUSE_URL: lighthouseUrl, WEBHOOK_SECRET: secret } = process.env;
@@ -73,8 +74,8 @@ class Runner {
 
     // If this run has specific lighthouse options, override base
     if (config && typeof config === 'object' && Object.keys(config).length) {
-      const { lhUrl, lhOptions } = this.parseConfig(config);
-      requestOptions = { ...requestOptions, lhOptions };
+      const { lhUrl, lhOptions } = parseConfig(config);
+      requestOptions = merge(requestOptions, lhOptions);
       // This allows a progressive switch to a custom LH endpoint
       if (lhUrl && lhUrl !== lighthouseUrl) {
         endpointUrl = lhUrl;

app/session.js

@@ -1,3 +1,4 @@
+const { homepage } = require('../package.json');
 const { getStats, processBudgets, processCategories, processLightWallet } = require('./budgets');
 const Status = require('./status');
 const Configuration = require('./configuration');
@@ -12,7 +13,8 @@ class Session {
     this.appParams = { appName };
     this.logger = logger;
     this.status = new Status(this.appParams);
-    this.configuration = new Configuration(this.appParams, this.status);
+    this.configHelp = `See [Configuration](${homepage}/#configuration) for help.`;
+    this.configuration = new Configuration(this.appParams, this.status, this.configHelp);
   }
 
   setVariables() {
@@ -46,22 +48,26 @@ class Session {
     const {
       baseUrl,
       ci: ciName,
-      type = 'check',
+      type = '',
       routes = [],
       settings = {},
-      namedSettings = {}
+      sharedSettings = {}
     } = await (config || this.configuration.getConfiguration());
 
     // return early if the config targets a different type
-    // this allows targeting deployments, or older `status` workflows.
+    // this allows targeting deployments, or older `status` workflows from the same provider.
     if (typeof isValid === 'function') {
-      const ciAppName = ciName.toLowerCase();
-      isValid = await isValid(type, [ciAppName, `${ciAppName}[bot]`]);
+      if (ciName && type) {
+        const ciAppName = ciName.toLowerCase();
+        isValid = await isValid(type, [ciAppName, `${ciAppName}[bot]`]);
+      } else {
+        isValid = false;
+      }
     }
 
     if (!baseUrl || !isValid) return;
 
-    // prepare variables since it's valid run
+    // prepare variables since it's a valid run
     this.setVariables();
 
     // Setup the runner, and exit if the url or token are empty
@@ -72,12 +78,23 @@ class Session {
       return;
     }
     // set up the url formatter
-    this.urlFormatter = urlFormatter(baseUrl, {
-      '{branch}': headBranch,
-      '{commit_hash}': headSha,
-      '{pr_number}': pullNumber,
-      ...macros
-    });
+    try {
+      this.urlFormatter = urlFormatter(baseUrl, {
+        '{branch}': headBranch,
+        '{commit_hash}': headSha,
+        '{pr_number}': pullNumber,
+        ...macros
+      });
+    } catch (error) {
+      await this.status.run({
+        conclusion: 'failure',
+        output: {
+          title: error.message,
+          summary: this.configHelp
+        }
+      });
+      return;
+    }
 
     // Setup routes or only run the baseUrl
     const urlRoutes =
@@ -111,14 +128,14 @@ class Session {
     this.logger.info('Started processing URLs...');
 
     this.settings = settings;
-    this.extendSettings = extendFromSettings(settings, namedSettings);
+    this.extendSettings = extendFromSettings(settings, sharedSettings);
 
     // Process each route and send a request
     await Promise.all(this.processRoutes(urlRoutes));
 
     this.logger.info('Finished processing URLs');
 
-    const { reportSummary = '', commentSummary = '', getTitle } = prepareReport(
+    const { reportSummary = '', commentSummary = '', getTitle, warningsFound } = prepareReport(
       this.order,
       this.reports
     );
@@ -136,14 +153,18 @@ class Session {
       return;
     }
 
+    if (warningsFound && this.conclusion === 'success') {
+      this.conclusion = 'neutral';
+    }
+
     // Attempt to post check and comment to Github
     try {
       await Promise.all([
         // github check
         status({
           conclusion: this.conclusion,
           output: {
-            title: getTitle(this.conclusion, this.errorsFound),
+            title: getTitle(this.conclusion, this.errorsFound, warningsFound),
             summary: reportSummary
           }
         }),
@@ -186,14 +207,14 @@ class Session {
 
     if (routeSettings) {
       try {
-        routeSettings = this.extendFromSettings(routeSettings);
+        routeSettings = this.extendSettings(routeSettings);
       } catch (err) {
         this.logger.error('There was an error processing the route settings', err);
         // push route, and add route to report
-        this.order.push(route);
-        this.reports.set(route, {
+        this.order.push(urlRoute);
+        this.reports.set(urlRoute, {
           report: detailsSummary(
-            `❌ <b>There was an error processing —</b> <i>${route}</i>`,
+            `❌ <b>There was an error processing —</b> <i>${urlRoute}</i>`,
             'Please check your configuration values'
           )
         });
@@ -220,7 +241,7 @@ class Session {
       this.reports.set(urlRoute, {
         report: detailsSummary(
           `❌ <b>No budgets were found in config for —</b> <i>${urlRoute}</i>`,
-          'Please include categories or budgets in settings'
+          `Please include categories or budgets in settings.\n${this.configHelp}`
         )
       });
       this.conclusion = 'failure';
@@ -257,7 +278,7 @@ class Session {
       Array.isArray(budgets) && processLightWallet(data.budgets, budgets, handleFailures)
     ]);
 
-    const lhVersion = `_Tested with Lighthouse Version: ${data.version}_`;
+    const lhVersion = `_Tested with Lighthouse Version: ${data.lighthouseVersion}_`;
     const reportUrl = data.reportUrl ? `Full Report: ${data.reportUrl}\n${lhVersion}` : lhVersion;
     // process the full report
     const report = detailsSummary(

app/settings.js

@@ -1,18 +1,18 @@
 const merge = require('lodash.merge');
 
-function extendFromSettings(baseSettings, namedSettings) {
+function extendFromSettings(baseSettings, sharedSettings) {
   return function extender({ extend, ...routeSettings }) {
     if (extend === true) {
       return merge({}, baseSettings, routeSettings);
     }
     if (typeof extend === 'string') {
       // attempts to get the shared setting, fallback to global
       // TODO: Throw error instead of fallback
-      const { extend: globalExtend, ...sharedSettings } = namedSettings[extend] || baseSettings;
+      const { extend: globalExtend, ...sharedSetting } = sharedSettings[extend] || baseSettings;
       if (globalExtend === true) {
-        return merge({}, baseSettings, sharedSettings, routeSettings);
+        return merge({}, baseSettings, sharedSetting, routeSettings);
       }
-      return merge({}, sharedSettings, routeSettings);
+      return merge({}, sharedSetting, routeSettings);
     }
 
     return routeSettings;

app/status.js

@@ -32,6 +32,32 @@ class Status {
     };
   }
 
+  /**
+   * Finds an existing check run from this application
+   */
+  async find() {
+    const { appName, context, github, headSha: ref } = this.params;
+    const {
+      data: { check_runs: checkRuns = [] }
+    } = await github.checks.listForRef(
+      context.repo({
+        ref
+      })
+    );
+
+    let checkRunOutput = {};
+
+    checkRuns.some(({ name, ...checkRun }) => {
+      if (name === appName) {
+        checkRunOutput = checkRun;
+        return true;
+      }
+      return false;
+    });
+
+    return checkRunOutput;
+  }
+
   /**
    * Adds/updates a check run.
    * @param {object} data The status options

app/util/index.js

@@ -52,7 +52,7 @@ async function getPullRequestNumber(github, headSha) {
  * @param {string} typeCheck The string to check against
  * @param {function} checkFunction An optional function to override the equal check
  */
-function isValidCheck(namesToCheck = [], typeCheck = 'check', checkFunction) {
+function isValidCheck(namesToCheck = [], typeCheck = '', checkFunction) {
   const checker = typeof checkFunction === 'function' ? checkFunction : false;
   return (type, ciNames) => {
     const valid = namesToCheck.filter(checkName => {
@@ -73,7 +73,7 @@ function isValidCheck(namesToCheck = [], typeCheck = 'check', checkFunction) {
  */
 function replaceMacros(keyMap) {
   const regexKeys = Object.keys(keyMap);
-  regexKeys.push('{commit_hash:(d)}');
+  regexKeys.push('{commit_hash:(\\d)}');
 
   return url => {
     const rgxp = new RegExp(regexKeys.join('|'), 'gi');