fix(server): usability improvements to PSI cron

Author: patrickhulce

packages/cli/src/collect/collect.js

@@ -42,10 +42,6 @@ function buildCommand(yargs) {
     psiApiKey: {
       description: '[psi only] The API key to use for PageSpeed Insights runner method.',
     },
-    psiApiEndpoint: {
-      description:
-        "[psi only] The endpoint to use for PageSpeed Insights runner method. You do not need to use this unless you've written a custom version.",
-    },
     staticDistDir: {
       description: 'The build directory where your HTML files to run Lighthouse on are located.',
     },

packages/server/src/cron/psi-collect.js

@@ -41,7 +41,7 @@ function normalizeCronSchedule(schedule) {
  * @return {Promise<void>}
  */
 async function psiCollectForProject(storageMethod, psi, site) {
-  const {urls, projectSlug, numberOfRuns = 3} = site;
+  const {urls, projectSlug, numberOfRuns = 5} = site;
   const project = await storageMethod.findProjectBySlug(projectSlug);
   if (!project) throw new Error(`Invalid project slug "${projectSlug}"`);
   if (!urls || !urls.length) throw new Error('No URLs set');
@@ -96,6 +96,13 @@ async function psiCollectForProject(storageMethod, psi, site) {
  */
 function startPsiCollectCron(storageMethod, options) {
   if (!options.psiCollectCron) return;
+  const uniqueProjectBranches = new Set(
+    options.psiCollectCron.sites.map(site => `${site.projectSlug}@${site.branch}`)
+  );
+
+  if (uniqueProjectBranches.size < options.psiCollectCron.sites.length) {
+    throw new Error('Cannot configure more than one cron per project-branch pair');
+  }
 
   /** @type {(msg: string) => void} */
   const log =

packages/server/test/cron/psi-collect.test.js

@@ -76,6 +76,8 @@ describe('cron/psi-collect', () => {
         [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
         [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
         [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
+        [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
+        [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
       ]);
       expect(storageMethod.sealBuild).toHaveBeenCalled();
     });
@@ -114,14 +116,13 @@ describe('cron/psi-collect', () => {
     });
 
     it('should respect number of runs', async () => {
-      const site = {urls: ['http://example.com'], numberOfRuns: 5};
+      const site = {urls: ['http://example.com'], numberOfRuns: 4};
       await psiCollectForProject(storageMethod, psi, site);
       expect(storageMethod.createRun.mock.calls).toMatchObject([
         [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
         [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
         [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
         [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
-        [{projectId: 1, buildId: 2, url: 'http://example.com', lhr: '{"lhr": true}'}],
       ]);
     });
 
@@ -137,21 +138,34 @@ describe('cron/psi-collect', () => {
     });
   });
 
-  describe('.startPsiCollectCron', () => {
+  describe('.startPsiCollectCron()', () => {
     const logLevel = 'silent';
 
     it('should schedule a cron job per site', () => {
       const psiCollectCron = {
         sites: [
-          {schedule: '0 * * * *', urls: ['http://example.com']},
-          {schedule: '0 * * * *', urls: ['http://other-example.com']},
+          {schedule: '0 * * * *', urls: ['http://example.com'], projectSlug: 'a'},
+          {schedule: '0 * * * *', urls: ['http://other-example.com'], projectSlug: 'b'},
         ],
       };
 
       startPsiCollectCron(storageMethod, {logLevel, psiCollectCron});
       expect(cronJob).toHaveBeenCalledTimes(2);
     });
 
+    it('should validate uniqueness', () => {
+      const psiCollectCron = {
+        sites: [
+          {schedule: '0 * * * *', urls: ['http://example.com']},
+          {schedule: '0 * * * *', urls: ['http://other-example.com']},
+        ],
+      };
+
+      expect(() => startPsiCollectCron(storageMethod, {logLevel, psiCollectCron})).toThrow(
+        /more than one/
+      );
+    });
+
     it('should validate cron job', () => {
       const psiCollectCron = {
         sites: [{schedule: '* * * * *', urls: ['http://example.com']}],

packages/utils/src/psi-client.js

@@ -33,6 +33,11 @@ class PsiClient {
     url.searchParams.set('locale', locale);
     url.searchParams.set('strategy', strategy);
     url.searchParams.set('key', this._apiKey);
+    url.searchParams.append('category', 'performance');
+    url.searchParams.append('category', 'accessibility');
+    url.searchParams.append('category', 'best-practices');
+    url.searchParams.append('category', 'pwa');
+    url.searchParams.append('category', 'seo');
 
     const response = await this._fetch(url.href);
     const body = await response.json();

packages/utils/test/psi-client.test.js

@@ -21,7 +21,7 @@ describe('PSI API Client', () => {
     const client = new PsiClient({apiKey, fetch: fetchMock});
     expect(await client.run('https://example.com')).toEqual(lighthouseResult);
     expect(fetchMock).toHaveBeenCalledWith(
-      'https://www.googleapis.com/pagespeedonline/v5/runPagespeed?url=https%3A%2F%2Fexample.com&locale=en_US&strategy=mobile&key=the-key'
+      'https://www.googleapis.com/pagespeedonline/v5/runPagespeed?url=https%3A%2F%2Fexample.com&locale=en_US&strategy=mobile&key=the-key&category=performance&category=accessibility&category=best-practices&category=pwa&category=seo'
     );
   });