Browse Source

ci: Update frontend file-filters to be each tailored to the tools they relate to (#67251)

Updates the file-filters lists to be more specific for each tool. There
are 3 tool categories: linting js (eslint and biome), linting css
(stylelint), testing files.

Note that each of these 3 categories includes a list of files to check,
but also a list of configs that should trigger a run too. So if someone
adds a new lint rule to the config we'll re-run everything right away.
Ryan Albrecht 11 months ago
parent
commit
78d41d0bf9

+ 0 - 1
.eslintignore

@@ -2,5 +2,4 @@
 **/vendor/**/*
 **/tests/**/fixtures/**/*
 !.github
-!.github/workflows/scripts/*
 *.d.ts

+ 63 - 38
.github/file-filters.yml

@@ -1,44 +1,69 @@
 # This is used by the action https://github.com/dorny/paths-filter
 
-# TODO: There are some meta files that we potentially could ignore for both front/backend,
-# as well as some configuration files that should trigger both
-frontend_components_lintable: &frontend_components_lintable
-  - '**/*.[tj]{s,sx}'
-
-frontend_lintable: &frontend_lintable
-  - *frontend_components_lintable
-  - '**/tsconfig*.json'
-  - '{package,now,vercel}.json'
-
-yarn_lockfile: &yarn_lockfile
-  - 'yarn.lock'
-
-eslint_config: &eslint_config
-  - '.eslint*'
-
-# `frontend_src` filters on files that are frontend changes excluding
-# changes to the tests/ directory.
-# If you want to filter on *all* frontend files, use the `frontend_all` filter.
-frontend_src: &frontend_src
-  - *yarn_lockfile
-  - *eslint_config
-  - '!(tests)/**/*.[tj]{s,sx}'
-  - '**/tsconfig*.json'
-  - '{package,now,vercel}.json'
-  - '**/*.less'
-  - 'static/**'
-  - 'fixtures/search-syntax/**/*'
-  - '.github/workflows/frontend.yml'
-
+sentry_specific_workflow: &sentry_specific_workflow
+  - added|modified: '.github/workflows/frontend.yml'
+sentry_specific_test_files: &sentry_specific_test_files
+  - added|modified: 'tests/js/**/*'
+  - added|deleted|modified: 'fixtures/search-syntax/**/*'
+  - added|deleted|modified: 'fixtures/js-stubs/**/*'
+
+# Provides list of changed app files to lint against (stylelint)
+lintable_css_in_js_modified: &lintable_css_in_js_modified
+  - added|modified: '**/*.[tj]{s,sx}'
+
+# Trigger for when we must run full lint (stylelint)
+lintable_css_in_js_rules_changed: &lintable_css_in_js_rules_changed
+  - *sentry_specific_workflow
+  - added|modified: '.github/file-filters.yml'
+  - added|modified: 'yarn.lock'
+  - added|deleted|modified: 'stylelint.*'
+
+# Provides list of changed files to lint against (eslint)
+lintable_modified: &lintable_modified
+  - added|modified: '**/*.[tj]{s,sx}'
+  - added|modified: '**/*.{html,less,json,yml,md}'
+
+# Trigger for when we must run full lint (eslint)
+lintable_rules_changed: &lintable_rules_changed
+  - *sentry_specific_workflow
+  - added|modified: '.github/file-filters.yml'
+  - added|modified: 'package.json'
+  - added|modified: 'yarn.lock'
+  - added|deleted|modified: '.prettierignore'
+  - added|deleted|modified: '.eslintignore'
+  - added|deleted|modified: '.eslint*'
+
+# Provides list of changed files to test (jest)
+# getsentry/sentry does not use this directly, instead we shard tests inside jest.config.js
+testable_modified: &testable_modified
+  - added|modified: 'package.json'
+  - added|modified: 'static/**/*.[tj]{s,sx}'
+  - *sentry_specific_test_files
+
+# Trigger for when we must run full tests (jest)
+testable_rules_changed: &testable_rules_changed
+  - *sentry_specific_workflow
+  - added|modified: '.github/file-filters.yml'
+  - added|modified: 'jest.config.ts'
+
+# Trigger whether to run tsc or not
+# There's no "rules_changed" for this, because we run it for all files always
+# getsentry/sentry does not use this directly, instead frontend_all is a superset to trigger tsc
+typecheckable_rules_changed: &typecheckable_rules_changed
+  - *sentry_specific_workflow
+  - added|modified: '.github/file-filters.yml'
+  - added|deleted|modified: '**/tsconfig*.json'
+  - added|deleted|modified: 'static/**/*.[tj]{s,sx}'
+
+# Trigger to apply the 'Scope: Frontend' label to PRs
 frontend_all: &frontend_all
-  - *frontend_src
-  - '**/*.[tj]{s,sx}'
-
-frontend_modified_lintable:
-  - added|modified: *frontend_lintable
-
-frontend_components_modified_lintable:
-  - added|modified: *frontend_components_lintable
+  - *lintable_css_in_js_rules_changed
+  - *lintable_rules_changed
+  - *testable_rules_changed
+  - *typecheckable_rules_changed
+  - added|modified: '{.volta,vercel}.json'
+  - *sentry_specific_workflow
+  - *sentry_specific_test_files
 
 # Also used in `getsentry-dispatch.yml` to dispatch backend tests on getsentry
 backend_dependencies: &backend_dependencies

+ 46 - 38
.github/workflows/frontend.yml

@@ -24,12 +24,16 @@ jobs:
     timeout-minutes: 3
     # Map a step output to a job output
     outputs:
-      eslint_config: ${{ steps.changes.outputs.eslint_config }}
-      frontend: ${{ steps.changes.outputs.frontend_all }}
-      frontend_components_modified_lintable: ${{ steps.changes.outputs.frontend_components_modified_lintable }}
-      frontend_components_modified_lintable_files: ${{ steps.changes.outputs.frontend_components_modified_lintable_files }}
-      frontend_modified_lintable_files: ${{ steps.changes.outputs.frontend_modified_lintable_files }}
-      yarn_lockfile: ${{ steps.changes.outputs.yarn_lockfile }}
+      lintable_css_in_js_modified: ${{ steps.changes.outputs.lintable_css_in_js_modified }}
+      lintable_css_in_js_modified_files: ${{ steps.changes.outputs.lintable_css_in_js_modified_files }}
+      lintable_css_in_js_rules_changed: ${{ steps.changes.output.lintable_css_in_js_rules_changed }}
+      lintable_modified: ${{ steps.changes.outputs.lintable_modified }}
+      lintable_modified_files: ${{ steps.changes.outputs.lintable_modified_files }}
+      lintable_rules_changed: ${{ steps.changes.outputs.lintable_rules_changed }}
+      testable_modified: ${{ steps.changes.outputs.testable_modified }}
+      testable_modified_files: ${{ steps.changes.outputs.testable_modified_files }}
+      testable_rules_changed: ${{ steps.changes.outputs.testable_rules_changed }}
+      frontend_all: ${{ steps.changes.outputs.frontend_all }}
     steps:
       - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
 
@@ -42,7 +46,7 @@ jobs:
           list-files: shell
 
   typescript-and-lint:
-    if: needs.files-changed.outputs.frontend == 'true'
+    if: needs.files-changed.outputs.frontend_all == 'true'
     needs: files-changed
     name: typescript and lint
     runs-on: ubuntu-22.04
@@ -70,49 +74,52 @@ jobs:
           echo "::add-matcher::.github/tsc.json"
           echo "::add-matcher::.github/eslint-stylish.json"
 
-      - name: eslint logic
-        id: eslint
-        if: (github.ref == 'refs/heads/master' || needs.files-changed.outputs.eslint_config == 'true' || needs.files-changed.outputs.yarn_lockfile == 'true')
-        run: echo "all-files=true" >> "$GITHUB_OUTPUT"
-
-      # Lint entire frontend if:
-      # - this is on main branch
-      # - eslint configuration in repo has changed
-      # - yarn lockfile has changed (i.e. we bump our eslint config)
-      - name: eslint
-        if: steps.eslint.outputs.all-files == 'true'
-        env:
-          # Run relax config on main branch (and stricter config for changed files)
-          SENTRY_ESLINT_RELAXED: 1
-        run: yarn lint
+      # When we're on master we can run all tasks across all files
+      # or if lint rules have changed, run the related task across all files
+      - name: biome (all files)
+        if: github.ref == 'refs/heads/master'
+        run: yarn lint:biome
+
+      - name: prettier (all files)
+        if: github.ref == 'refs/heads/master' || needs.files-changed.outputs.lintable_rules_changed == 'true'
+        run: yarn lint:prettier
+
+      - name: eslint (all files)
+        if: github.ref == 'refs/heads/master' || needs.files-changed.outputs.lintable_rules_changed == 'true'
+        run: yarn lint:js
+
+      - name: stylelint (all files)
+        if: github.ref == 'refs/heads/master' || needs.files-changed.outputs.lintable_css_in_js_rules_changed == 'true'
+        run: yarn lint:css
+
+      # When on a PR branch, we only need to look at the changed files
+      - name: biome (fix)
+        if: github.ref != 'refs/heads/master'
+        run: yarn fix:biome
+
+      - name: prettier (changed files only)
+        if: github.ref != 'refs/heads/master' && needs.files-changed.outputs.lintable_rules_changed != 'true' && needs.files-changed.outputs.lintable_modified == 'true'
+        run: yarn prettier --write ${{ needs.files-changed.outputs.lintable_modified_files }}
 
-      # Otherwise... only lint modified files
-      # Note `eslint --fix` will not fail when it auto fixes files
       - name: eslint (changed files only)
-        if: steps.eslint.outputs.all-files != 'true'
-        run: |
-          yarn eslint --fix ${{ needs.files-changed.outputs.frontend_modified_lintable_files }}
+        if: github.ref != 'refs/heads/master' && needs.files-changed.outputs.lintable_rules_changed != 'true' && needs.files-changed.outputs.lintable_modified == 'true'
+        run: yarn eslint --fix ${{ needs.files-changed.outputs.lintable_modified_files }}
 
       - name: stylelint (changed files only)
-        if: github.ref != 'refs/heads/master' && needs.files-changed.outputs.frontend_components_modified_lintable == 'true'
-        run: |
-          yarn stylelint ${{ needs.files-changed.outputs.frontend_components_modified_lintable_files }}
-
-      - name: biome
-        if: github.ref != 'refs/heads/master' && needs.files-changed.outputs.frontend_components_modified_lintable == 'true'
-        run: yarn biome check . --apply
+        if: github.ref != 'refs/heads/master' && needs.files-changed.outputs.lintable_css_in_js_rules_changed != 'true' && needs.files-changed.outputs.lintable_css_in_js_modified == 'true'
+        run: yarn stylelint ${{ needs.files-changed.outputs.lintable_css_in_js_modified_files }}
 
       # Check (and error) for dirty working tree for forks
       # Reason being we need a different token to auto commit changes and
       # forks do not have access to said token
       - name: Check for dirty git working tree (forks)
-        if: steps.token.outcome != 'success' && github.ref != 'refs/heads/master'
+        if: github.ref != 'refs/heads/master' && steps.token.outcome != 'success'
         run: |
           git diff --quiet || (echo '::error ::lint produced file changes, run linter locally and try again' && exit 1)
 
       # If working tree is dirty, commit and update if we have a token
       - name: Commit any eslint fixed files
-        if: steps.token.outcome == 'success' && github.ref != 'refs/heads/master'
+        if: github.ref != 'refs/heads/master' && steps.token.outcome == 'success'
         uses: getsentry/action-github-commit@31f6706ca1a7b9ad6d22c1b07bf3a92eabb05632 # v2.0.0
         with:
           github-token: ${{ steps.token.outputs.token }}
@@ -124,7 +131,7 @@ jobs:
         run: yarn tsc -p config/tsconfig.ci.json
 
   frontend-jest-tests:
-    if: needs.files-changed.outputs.frontend == 'true'
+    if: needs.files-changed.outputs.testable_rules_changed == 'true' || needs.files-changed.outputs.testable_modified == 'true'
     needs: files-changed
     name: Jest
     # If you change the runs-on image, you must also change the runner in jest-balance.yml
@@ -137,6 +144,7 @@ jobs:
       fail-fast: false
       matrix:
         # XXX: When updating this, make sure you also update CI_NODE_TOTAL.
+
         instance: [0, 1, 2, 3]
 
     steps:
@@ -186,7 +194,7 @@ jobs:
       # it reduces large coverage fluctuations.
       - name: Handle artifacts
         uses: ./.github/actions/artifacts
-        if: ${{ always() && needs.files-changed.outputs.frontend_all == 'true' }}
+        if: always()
         with:
           files: .artifacts/coverage/*
           type: frontend

+ 2 - 2
.github/workflows/label-pullrequest.yml

@@ -25,7 +25,7 @@ jobs:
 
       - name: Add frontend label
         uses: getsentry/action-add-labels@54d0cba498c1eaf8bd34985d715504d1b6e2935f
-        if: steps.changes.outputs.frontend_src == 'true'
+        if: steps.changes.outputs.frontend_all == 'true'
         with:
           labels: 'Scope: Frontend'
 
@@ -46,7 +46,7 @@ jobs:
       - name: Add frontend/backend warning comment
         uses: peter-evans/create-or-update-comment@b95e16d2859ad843a14218d1028da5b2c4cbc4b4
         if: >
-          steps.changes.outputs.frontend_src == 'true' &&
+          steps.changes.outputs.frontend_all == 'true' &&
           steps.changes.outputs.backend_src == 'true' &&
           steps.fc.outputs.comment-id == 0
         with:

+ 3 - 3
package.json

@@ -223,12 +223,12 @@
     "test-precommit": "yarn test-wrap --bail --findRelatedTests -u",
     "test-staged": "yarn test-wrap --findRelatedTests $(git diff --name-only --cached)",
     "lint": "yarn lint:biome && yarn lint:prettier && yarn lint:js && yarn lint:css",
-    "lint:js": "eslint static/app tests/js --ext .js,.jsx,.ts,.tsx",
-    "lint:css": "stylelint 'static/app/**/*.[jt]sx'",
+    "lint:js": "eslint . --ext .js,.jsx,.ts,.tsx",
+    "lint:css": "stylelint '**/*.[jt]sx'",
     "lint:biome": "biome check .",
     "lint:prettier": "prettier \"**/*.md\" \"**/*.yaml\" \"**/*.[jt]s(x)?\" --check",
     "fix": "yarn fix:biome && yarn fix:prettier && yarn fix:eslint",
-    "fix:eslint": "eslint static/app tests/js --ext .js,.jsx,.ts,.tsx --fix",
+    "fix:eslint": "eslint . --ext .js,.jsx,.ts,.tsx --fix",
     "fix:biome": "biome check . --apply",
     "fix:prettier": "prettier \"**/*.md\" \"**/*.yaml\" \"**/*.[jt]s(x)?\" --write",
     "dev": "(yarn check --verify-tree || yarn install --check-files) && sentry devserver",

+ 1 - 1
scripts/extract-android-device-names.js

@@ -3,7 +3,7 @@ const fs = require('node:fs');
 
 const transformResults = res => {
   const deviceMapping = {};
-  res.map(({name, model}) => {
+  res.forEach(({name, model}) => {
     if (name && model) {
       deviceMapping[model] = name;
     }

+ 0 - 1
scripts/test.js

@@ -13,7 +13,6 @@ process.on('unhandledRejection', err => {
   throw err;
 });
 
-// eslint-disable-next-line jest/no-jest-import
 const jest = require('jest');
 
 let argv = process.argv.slice(2);