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

[CI] Lighthouse updates #1632

Closed
andrewetchen opened this issue Apr 19, 2022 · 4 comments · Fixed by #1898
Closed

[CI] Lighthouse updates #1632

andrewetchen opened this issue Apr 19, 2022 · 4 comments · Fixed by #1898
Assignees

Comments

@andrewetchen
Copy link
Contributor

Reminders for me to look into the following:

Lighthouse CI

We are currently a deprecated authentication config for our Lighthouse CI job: app_id and app_password are deprecated.

lhci:
name: Lighthouse
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Lighthouse
uses: shopify/lighthouse-ci-action@1.0
with:
app_id: ${{ secrets.SHOP_APP_ID }}
app_password: ${{ secrets.SHOP_APP_PASSWORD }}
store: ${{ secrets.SHOP_STORE }}
password: ${{ secrets.SHOP_PASSWORD }}
lhci_github_app_token: ${{ secrets.LHCI_GITHUB_TOKEN }}

https://github.com/Shopify/lighthouse-ci-action/blob/13cf9a620e4d97fb36b2e5a87dae2eefc2670f21/action.yml#L43-L50

...
  app_id:
    description: '(deprecated) Shopify private app ID'
    depreciationMessage: Shopify private apps are deprecated. Use a Custom App `access_token` instead.
    required: false
  app_password:
    description: '(deprecated) Shopify private app password'
    depreciationMessage: Shopify private apps are deprecated. Use a Custom App `access_token` instead.
    required: false
...

We should use the Custom app authentication method.


Add leaving v by changing shopify/lighthouse-ci-action@1.0 to shopify/lighthouse-ci-action@v1: this might not be too critical as Github doesn't currently apply any versioning semantics for actions but they might in the future and its good practice.

uses: shopify/lighthouse-ci-action@1.0

This was also recently changed on the lighthouse-ci-action repo.

I tested this by pushing a test commit and let the CI do its thing:

@andrewetchen andrewetchen self-assigned this Apr 19, 2022
@andrewetchen
Copy link
Contributor Author

andrewetchen commented Apr 25, 2022

Update April 25 2022

To use the Custom app method for authentication, I added a custom app to our store.

Next steps

Add SHOP_ACCESS_TOKEN as a new secret and update the following section of our ci.yml:

From this:

...
  with:
    app_id: ${{ secrets.SHOP_APP_ID }}
    app_password: ${{ secrets.SHOP_APP_PASSWORD }}
    store: ${{ secrets.SHOP_STORE }}
    password: ${{ secrets.SHOP_PASSWORD }}
    lhci_github_app_token: ${{ secrets.LHCI_GITHUB_TOKEN }}

To this:

...
  with:
    store: ${{ secrets.SHOP_STORE }}
    password: ${{ secrets.SHOP_PASSWORD }}
    access_token: ${{ secrets.SHOP_ACCESS_TOKEN }}
    lhci_github_app_token: ${{ secrets.LHCI_GITHUB_TOKEN }}

@andrewetchen
Copy link
Contributor Author

andrewetchen commented Apr 29, 2022

Update

I coordinated with Tyler to get the new secrets added to the repo and everything seems to be good to go in terms of using the non-deprecated authentication method.

I'll have to do some more digging but I wanted to quickly point out that we might've been getting skewed Lighthouse scores using the deprecated authentication method (how it is currently):

This was an action run from a recent commit:

Here’s an action run using the new Custom app authentication method:

@andrewetchen
Copy link
Contributor Author

Update

I ran 2 more tests, pushing 2 separate commits:

The os2-demo accessibility report shows why it’s failing:

  • I've confirmed that a stores' live or unpublished themes do not factor into Lighthouse CI tests.
  • What does play a part is how each store (dawn-dev-ci and os2-demo) is setup differently (Products, Navigation, Collections, etc.).
  • When the Lighthouse CI starts, it runs on the code of the PR and "pushes" it to a non-published/unlisted development theme (not shown in Admin > Themes) and is tested against this.

@andrewetchen
Copy link
Contributor Author

andrewetchen commented May 16, 2022

Update - May 16 2022

Adding some notes. I'm hoping this will also help establish a Lighthouse CI standard for our other themes, not just Dawn.

Our current Lighthouse CI runs proposed changes against the dawn-dev-ci store: if you create a PR with changes to the header.liquid, the Lighthouse CI will take a copy of this version of Dawn and test against the store setup/config of dawn-dev-ci.

Navigation / Header

Running the Lighthouse CI on os2-demo will result in failed accessibility audit (noted in above comment).

  • Upon further troubleshooting, one of the main differences between dawn-dev-ci and os2-demo is the navigation menus
    • Screenshot of dawn-dev-ci navigation
    • The os2-demo navigation has a lot more menu items but the thing to highlight here is that when anchor links are used, the ARIA IDs are no longer unique. This is what causes the Lighthouse CI audit to fail accessibility: screenshot
    • Tyler mentioned that we'd need to update the header.liquid by assigning a parent index:
      • 5e7c1b7
      • These changes resulted in a passed Lighthouse accessibility audit.
      • Another way to think of about this is that all current and previous Lighthouse CI runs (especially PRs with changes to the header, navigation, etc.) weren't being checked effectively since dawn-dev-ci only has 2 menu items: we weren't checking for nested menu items and anchors etc.

Lighthouse CI / ci.yml improvements

When we do update the Shopify/dawn repo to use the os2-demo store instead of dawn-dev-ci, we should take advantage of the following options (note: the Lighthouse CI runs 3 times on the homepage, product page, and collection page):

We can also look into Apps to see if any of them affect the Lighthouse CI runs (installed and not enabled: think Shopify Product Reviews).

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 a pull request may close this issue.

1 participant