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

fix(plugin-lighthouse): add chromium to dev deps #654

Merged
merged 13 commits into from
May 2, 2024
Merged

Conversation

BioPhoton
Copy link
Collaborator

@BioPhoton BioPhoton commented Apr 25, 2024

This PR adds chromium as NPM dependency to the repository. This should enable a development setup that works after npm ci out of the box. Potential further steps are adding chrome to plugin-lighthosue too.

This PR includes:

  • new dev dependency chromium
  • remove the chrome path handling for windows CI

Note

Our testing setup snippet uses getChromePath which internally does not check for chromium, so I added a fallback.
See: testing/test-setup/src/lib/chrome-path.setup.ts

How to test it for contributors that did not have Chrome installed?

On your local machine:

  1. make sure you have no value set for CHROME_PATH in the env variables (optional)
  2. ✅ run e2e tests and have them executed correctly
  3. 🗑️ deinstall chrome
  4. ❌ run e2e tests and have them failing because chrome not found
  5. ⬇️ check out this branch
  6. ⏳ run npm ci
  7. ✅ run e2e tests and have them executed correctly

How to test it for contributors that had Chrome installed but autodetecting the path failed?

On your local machine:

  1. make sure you have no value set for CHROME_PATH in the env variables (optional)
  2. ⬇️ check out this branch
  3. ✅ run e2e tests and have them executed correctly
  4. 🗑️ deinstall chromium npm remove chromium
  5. ❌ run e2e tests and have them failing because chrome not found

closes #650

@github-actions github-actions bot added 📖 Project documentation improvements or additions to the project documentation 🛠️ tooling labels Apr 25, 2024
Copy link

nx-cloud bot commented Apr 25, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6c264e4. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 6 targets

Sent with 💌 from NxCloud.

Copy link

github-actions bot commented Apr 25, 2024

Code PushUp

🤨 Code PushUp report has both improvements and regressions – compared target commit db87192 with source commit f11f8a7.

🏷️ Categories

🏷️ Category ⭐ Current score ⭐ Previous score 🔄 Score change
Performance 🟡 59 🟡 85 ↓ −25.9
Code coverage 🟡 68 🟡 67 ↑ +0.9
Updates 🟡 70 🟡 69 ↑ +0.9
Accessibility 🟢 100 🟢 100
Best Practices 🟢 100 🟢 100
SEO 🟡 86 🟡 86
PWA 🟡 63 🟡 63
Bug prevention 🟢 100 🟢 100
Code style 🟢 99 🟢 99
Security 🟡 80 🟡 80
Custom checks 🟡 67 🟡 67

🗃️ Groups

👍 2 groups improved, 👎 1 group regressed
🔌 Plugin 🗃️ Group ⭐ Current score ⭐ Previous score 🔄 Score change
Lighthouse Performance 🟡 59 🟡 85 ↓ −25.9
Code coverage Code coverage metrics 🟡 68 🟡 67 ↑ +0.9
JS Packages NPM outdated dependencies 🟡 70 🟡 69 ↑ +0.9

18 other groups are unchanged.

🛡️ Audits

👍 2 audits improved, 👎 11 audits regressed, 10 audits changed without impacting score
🔌 Plugin 🛡️ Audit 📏 Current value 📏 Previous value 🔄 Value change
Lighthouse JavaScript execution time 🟥 1.6 s 🟩 0.8 s ↑ +88 %
Lighthouse Largest Contentful Paint element 🟥 3,080 ms 🟩 2,500 ms  −∞ %
Lighthouse Total Blocking Time 🟥 2,510 ms 🟨 490 ms ↑ +417 %
Lighthouse Speed Index 🟨 5.0 s 🟩 2.0 s ↑ +155 %
Lighthouse First Contentful Paint 🟨 2.4 s 🟩 1.5 s ↑ +53 %
Lighthouse Time to Interactive 🟨 5.5 s 🟩 3.3 s ↑ +64 %
Lighthouse First Meaningful Paint 🟨 2.8 s 🟩 1.6 s ↑ +75 %
Lighthouse Largest Contentful Paint 🟨 3.1 s 🟩 2.5 s ↑ +23 %
Lighthouse Max Potential First Input Delay 🟥 770 ms 🟥 440 ms ↑ +76 %
JS Packages Outdated NPM dev dependencies. 🟨 48 outdated package versions (24 major, 20 minor, 3 patch, 1 prerelease) 🟥 45 outdated package versions (25 major, 14 minor, 6 patch) ↑ +7 %
Code coverage Function coverage 🟨 61 % 🟨 59 % ↑ +3 %
Code coverage Branch coverage 🟨 82 % 🟨 82 %  +0 %
Code coverage Line coverage 🟨 65 % 🟨 65 %  +0 %
Lighthouse Minimizes main-thread work 🟥 7.4 s 🟥 3.8 s ↑ +96 %
Lighthouse Metrics 🟩 5472 🟩 3331 ↑ +64 %
Lighthouse Properly size images 🟥 Potential savings of 327 KiB 🟥 Potential savings of 327 KiB ↓ −100 %
Lighthouse Avoids enormous network payloads 🟩 Total size was 467 KiB 🟩 Total size was 467 KiB ↓ +0 %
Lighthouse Uses efficient cache policy on static assets 🟨 14 resources found 🟨 14 resources found ↓ +0 %
Lighthouse Server Backend Latencies 🟩 40 ms 🟩 10 ms ↑ +529 %
Lighthouse Network Round Trip Times 🟩 10 ms 🟩 0 ms ↑ +500 %
Lighthouse Initial server response time was short 🟩 Root document took 10 ms 🟩 Root document took 0 ms ↑ +50 %
JS Packages Vulnerabilities for NPM dev dependencies. 🟥 24 vulnerabilities (24 moderate) 🟥 23 vulnerabilities (23 moderate) ↑ +4 %
JS Packages Outdated NPM prod dependencies. 🟩 5 outdated package versions (2 minor, 3 patch) 🟩 4 outdated package versions (2 minor, 2 patch) ↑ +25 %

500 other audits are unchanged.

@github-actions github-actions bot added the 🦾 CI/CD continuous integration and deployment label Apr 27, 2024
@BioPhoton BioPhoton changed the title fix(lighthouse-plugin): add chromium to dev deps fix(plugin-lighthouse): add chromium to dev deps Apr 27, 2024
@BioPhoton BioPhoton marked this pull request as ready for review April 27, 2024 15:20
@BioPhoton BioPhoton requested review from Tlacenka and getlarge April 27, 2024 15:45
getlarge
getlarge previously approved these changes Apr 27, 2024
Copy link
Collaborator

@getlarge getlarge left a comment

Choose a reason for hiding this comment

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

In the name of all the people not using Chrome, thank you :)

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@github-actions github-actions bot added the 🔬 testing writing tests label Apr 28, 2024
@BioPhoton BioPhoton requested review from matejchalk and removed request for Tlacenka April 29, 2024 07:36
vmasek
vmasek previously approved these changes Apr 29, 2024
Copy link
Collaborator

@vmasek vmasek left a comment

Choose a reason for hiding this comment

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

👍

packages/plugin-lighthouse/CONTRIBUTING.md Outdated Show resolved Hide resolved
packages/plugin-lighthouse/CONTRIBUTING.md Outdated Show resolved Hide resolved
packages/plugin-lighthouse/CONTRIBUTING.md Outdated Show resolved Hide resolved
testing/test-setup/src/lib/chrome-path.setup.ts Outdated Show resolved Hide resolved
vmasek
vmasek previously approved these changes Apr 30, 2024
Copy link
Collaborator

@vmasek vmasek left a comment

Choose a reason for hiding this comment

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

✅Did a check on fresh windows installation without Chrome.

On main branch it complains about no path to Chrome.exe,
on this branch all integration tests pass and chromium path is autodetected as a local path \node_modules\chromium\lib\chromium\chrome-win\chrome.exe

@BioPhoton BioPhoton merged commit 6f92d90 into main May 2, 2024
19 checks passed
@BioPhoton BioPhoton deleted the add-dev-dept branch May 2, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🦾 CI/CD continuous integration and deployment 📖 Project documentation improvements or additions to the project documentation 🔬 testing writing tests 🛠️ tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implicit dependency on Chrome
4 participants