Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add function to add rand={randomNumber} to the urls to inspect #2

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

batbattur
Copy link
Contributor

@batbattur batbattur commented Sep 12, 2023

Summary

This adds rand={randomNumber} query param to the urls.
This is to ensure that we never hit CDN cache.

QA notes

  1. pnpm install
  2. Add the following urls.json file:
{
    "Test": ["https://www.google.com/", "https://www.youtube.com"],
    "Testing": ["https://www.google.com/?raasdasd", "https://www.youtube.com?asdasd"]
}
  1. Add the following diff:
diff --git a/src/index.ts b/src/index.ts
index 16f710c..b6fccfa 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -103,20 +103,5 @@ async function captureLighthouseMetrics(pageType: string, url: string, audits: s
 (async() => {
     const inspectList: Record<string, string[]> = JSON.parse(await fs.promises.readFile('urls.json', 'utf8'));
     const updatedInspectList: Record<string, string[]> = addRandomParamToUrl(inspectList);
+    console.log(updatedInspectList)
-    const coreMetrics: Record<string, string[]> = JSON.parse(await fs.promises.readFile('metrics-config.json', 'utf8'));
-
-    for (let [pageType, urls] of Object.entries(updatedInspectList)) {
-        console.log(`Capturing metrics for ${pageType} page(s)\n`)
-
-        for (let url of urls) {
-            await captureLighthouseMetrics(pageType, url, coreMetrics.audits, {}, lhDesktopConfig)
-
-            await captureLighthouseMetrics(pageType, url, coreMetrics.audits, {}, lhMobileConfig)
-        }
-
-        console.log(`Done capturing metrics for ${pageType} page(s)\n`)
-    }
-
-    console.log('Done capturing metrics for all pages')
  1. pnpm run build
  2. pnpm start
  3. Make sure the rand param is added to the urls when logging updatedInspectList.

closes https://github.com/iFixit/server-templates/issues/4140

@batbattur batbattur self-assigned this Sep 12, 2023
Copy link
Contributor

@ardelato ardelato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR 👍 but deploy_block ☁️ with some questions

If the provided urls.json file is bad, it exits early on JSON.parse()
and doesn't get to this check, thus, dropping it.
Copy link
Contributor

@ardelato ardelato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR 👍

Used structuredClone() to clone the inital inspectList. The `structuredClone()`
is avaliable on node +17 versions, thus, updated the node version constraint
in package.json.
ardelato
ardelato previously approved these changes Sep 12, 2023
Copy link
Contributor

@ardelato ardelato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR 👍 and un_deploy_block ⚡

@erinemay erinemay added the QAing Under QA team review label Sep 13, 2023
@erinemay
Copy link

QA 🍰
image

The param is added appropriately as the first url param or a subsequent url param

@ardelato
Copy link
Contributor

deploy_block ☁️ on #1 going out first

@erinemay erinemay removed the QAing Under QA team review label Sep 13, 2023
@ardelato ardelato changed the base branch from port-lighthouse-docker to main September 13, 2023 23:16
@ardelato ardelato dismissed their stale review September 13, 2023 23:16

The base branch was changed.

@batbattur
Copy link
Contributor Author

QA :carry-over:
Screen Shot 2023-09-13 at 4 19 43 PM

Copy link
Contributor

@ardelato ardelato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR 👍

@ardelato
Copy link
Contributor

un_deploy_block ⚡ #1 has been merged.

@ardelato ardelato merged commit 897e888 into main Sep 13, 2023
@ardelato ardelato deleted the add-query-param-to-urls branch September 13, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants