Merge pull request 'ci/tests(e2e): always run e2e tests, but only on changed files' (#5450) from fnetx/e2e-on-changes into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5450
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

.forgejo/workflows-composite/build-backend/action.yaml

@@ -6,7 +6,7 @@ runs:
     - uses: actions/cache@v4
       id: cache-backend
       with:
-        path: '/workspace/forgejo/forgejo/gitea'
+        path: ${{github.workspace}}/gitea
         key: backend-build-${{ github.sha }}
     - if: steps.cache-backend.outputs.cache-hit != 'true'
       run: |

.forgejo/workflows-composite/setup-env/action.yaml

@@ -4,7 +4,8 @@ runs:
     - name: setup user and permissions
       run: |
         git config --add safe.directory '*'
-        adduser --quiet --comment forgejo --disabled-password forgejo
+        # ignore if the user already exists (like with the playwright image)
+        adduser --quiet --comment forgejo --disabled-password forgejo || true
         chown -R forgejo:forgejo .
     - uses: https://codeberg.org/fnetx/setup-cache-go@b2214eaf6fb44c7e8512c0f462a2c3ec31f86a73
       with:

.forgejo/workflows/e2e.yml

@@ -1,37 +0,0 @@
-name: e2e
-
-on:
-  pull_request:
-    paths:
-      - Makefile
-      - playwright.config.js
-      - .forgejo/workflows/e2e.yml
-      - tests/e2e/**
-      - web_src/js/**
-      - web_src/css/form.css
-      - templates/webhook/shared-settings.tmpl
-      - templates/org/team/new.tmpl
-
-jobs:
-  test-e2e:
-    if: ${{ !startsWith(vars.ROLE, 'forgejo-') }}
-    runs-on: docker
-    container:
-      image: 'code.forgejo.org/oci/playwright:latest'
-    steps:
-      - uses: https://code.forgejo.org/actions/checkout@v4
-      - uses: https://code.forgejo.org/actions/setup-go@v5
-        with:
-          go-version-file: "go.mod"
-      - run: |
-          git config --add safe.directory '*'
-          chown -R forgejo:forgejo .
-      - run: |
-          su forgejo -c 'make deps-frontend frontend deps-backend'
-      - run: |
-          su forgejo -c 'make backend'
-      - run: |
-          su forgejo -c 'make generate test-e2e-sqlite'
-        timeout-minutes: 40
-        env:
-          USE_REPO_TEST_DIR: 1

.forgejo/workflows/testing.yml

@@ -36,6 +36,17 @@ jobs:
       - run: make checks-frontend
       - run: make test-frontend-coverage
       - run: make frontend
+      - name: Install zstd for cache saving
+        # works around https://github.com/actions/cache/issues/1169, because the
+        # consuming job has zstd and doesn't restore the cache otherwise
+        run: |
+          apt-get update -qq
+          apt-get -q install -qq -y zstd
+      - name: "Cache frontend build for playwright testing"
+        uses: actions/cache/save@v4
+        with:
+          path: ${{github.workspace}}/public/assets
+          key: frontend-build-${{ github.sha }}
   test-unit:
     if: ${{ !startsWith(vars.ROLE, 'forgejo-') }}
     runs-on: docker
@@ -75,10 +86,43 @@ jobs:
           RACE_ENABLED: 'true'
           TAGS: bindata
           TEST_ELASTICSEARCH_URL: http://elasticsearch:9200
-  test-remote-cacher:
+  test-e2e:
     if: ${{ !startsWith(vars.ROLE, 'forgejo-') }}
     runs-on: docker
     needs: [backend-checks, frontend-checks]
+    container:
+      image: 'code.forgejo.org/oci/playwright:latest'
+    steps:
+      - uses: https://code.forgejo.org/actions/checkout@v4
+        with:
+          fetch-depth: 20
+      - uses: ./.forgejo/workflows-composite/setup-env
+      - name: "Restore frontend build"
+        uses: actions/cache/restore@v4
+        id: cache-frontend
+        with:
+          path: ${{github.workspace}}/public/assets
+          key: frontend-build-${{ github.sha }}
+      - name: "Build frontend (if not cached)"
+        if: steps.cache-frontend.outputs.cache-hit != 'true'
+        run: |
+          su forgejo -c 'make deps-frontend frontend'
+      - uses: ./.forgejo/workflows-composite/build-backend
+      - name: Get changed files
+        id: changed-files
+        uses: https://code.forgejo.org/fossdd/changed-files@v45
+        with:
+          separator: '\n'
+      - run: |
+          su forgejo -c 'make generate test-e2e-sqlite'
+        timeout-minutes: 40
+        env:
+          USE_REPO_TEST_DIR: 1
+          CHANGED_FILES: ${{steps.changed-files.outputs.all_changed_files}}
+  test-remote-cacher:
+    if: ${{ !startsWith(vars.ROLE, 'forgejo-') }}
+    runs-on: docker
+    needs: [backend-checks, frontend-checks, test-unit]
     container:
       image: 'code.forgejo.org/oci/node:20-bookworm'
     strategy:

CODEOWNERS

@@ -16,6 +16,9 @@ templates/.* @caesar @crystal @gusted
 ## the issue sidebar was touched by fnetx
 templates/repo/issue/view_content/sidebar.* @fnetx
 
+# Playwright tests
+tests/e2e/.* @fnetx
+
 # Files related to Go development.
 
 # The modules usually don't require much knowledge about Forgejo and could

tests/e2e/README.md

@@ -110,9 +110,13 @@ If you have a [forgejo runner](https://code.forgejo.org/forgejo/runner/),
 you can use it to run the test jobs:
 
 ```
-forgejo-runner exec -W .forgejo/workflows/e2e.yml --event=pull_request
+forgejo-runner exec -W .forgejo/workflows/testing.yml -j test-e2e
 ```
 
+Note that the CI workflow has some logic to run tests based on changed files only.
+This might conflict with your local setup and not run all the desired tests
+because it might only look at file changes in your latest commit.
+
 ### Run e2e tests with another database
 
 This approach is not currently used,
@@ -212,9 +216,52 @@ Take a look at `shared/forms.js` and some other places for inspiration.
 
 ### List related files coverage
 
-If you think your playwright tests covers an important aspect of some template, CSS or backend files,
+To speed up the CI pipelines and avoid running expensive tests too often,
-consider adding the paths to `.forgejo/workflows/e2e.yml` in the path filter.
+only a selection of tests is run by default,
+based on the changed files.
 
-It ensures that future modifications to this file will be tested as well.
+At the top of each playwright test file,
+list the files or file patterns that are covered by your test.
+Often, these are files that you modified for your feature or bugfix,
+or that you looked at (and might still have open in your IDE),
+because your fix depends on their behaviour.
 
-Currently, we do not run the e2e tests on all changes.
+#### Which files to watch?
+
+The set of files your test "watches" depends on the kind of test you write.
+If you only test for the presence of an element and do no accessibility or placement checks,
+you won't detect broken visual appearance and there is little reason to watch CSS files.
+
+However, if your test also checks that an element is correctly positioned
+(e.g. that it does not overflow the page),
+or has accessibiltiy properties (includes colour contrast),
+also list stylesheets that define the behaviour your test depends on.
+
+Watching the place that generate the selectors you use
+(typically templates, but can also be JavaScript)
+is a must, to ensure that someone modifying the markup notices that your selectors fail
+(e.g. because an id or class was renamed).
+
+If you are unsure about the exact set of files,
+feel free to ask other contributors.
+
+#### How to specify the patterns?
+
+You put filenames and patterns as blocks between two `// @watch` comments.
+An example that watches changes on (in order)
+a single file,
+a full recursive subfolder,
+two files with a shorthand pattern,
+and a set of files with a certain ending:
+
+~~~
+// @watch start
+// templates/webhook/shared-settings.tmpl
+// templates/repo/settings/**
+// web_src/css/{form,repo}.css
+// web_src/css/modules/*.css
+// @watch end
+~~~
+
+The patterns are evaluated on a "first-match" basis.
+Under the hood, [gobwas/glob](https://github.com/gobwas/glob) is used.

tests/e2e/actions.test.e2e.js

@@ -1,4 +1,16 @@
 // @ts-check
+
+// @watch start
+// templates/repo/actions/**
+// web_src/css/actions.css
+// web_src/js/components/ActionRunStatus.vue
+// web_src/js/components/RepoActionView.vue
+// modules/actions/**
+// modules/structs/workflow.go
+// routers/api/v1/repo/action.go
+// routers/web/repo/actions/**
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, load_logged_in_context} from './utils_e2e.js';
 

tests/e2e/changes.go

@@ -0,0 +1,114 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package e2e
+
+import (
+	"bufio"
+	"os"
+	"strings"
+
+	"code.gitea.io/gitea/modules/log"
+
+	"github.com/gobwas/glob"
+)
+
+var (
+	changesetFiles     []string
+	changesetAvailable bool
+	globalFullRun      bool
+)
+
+func initChangedFiles() {
+	var changes string
+	changes, changesetAvailable = os.LookupEnv("CHANGED_FILES")
+	// the output of the Action seems to actually contain \n and not a newline literal
+	changesetFiles = strings.Split(changes, `\n`)
+	log.Info("Only running tests covered by a subset of test files. Received the following list of CHANGED_FILES: %q", changesetFiles)
+
+	globalPatterns := []string{
+		// meta and config
+		"Makefile",
+		"playwright.config.js",
+		".forgejo/workflows/testing.yml",
+		"tests/e2e/*.go",
+		"tests/e2e/shared/*",
+		// frontend files
+		"frontend/*.js",
+		"frontend/{base,index}.css",
+		// templates
+		"templates/base/**",
+	}
+	fullRunPatterns := []glob.Glob{}
+	for _, expr := range globalPatterns {
+		fullRunPatterns = append(fullRunPatterns, glob.MustCompile(expr, '.', '/'))
+	}
+	globalFullRun = false
+	for _, changedFile := range changesetFiles {
+		for _, pattern := range fullRunPatterns {
+			if pattern.Match(changedFile) {
+				globalFullRun = true
+				log.Info("Changed files match global test pattern, running all tests")
+				return
+			}
+		}
+	}
+}
+
+func canSkipTest(testFile string) bool {
+	// run all tests when environment variable is not set or changes match global pattern
+	if !changesetAvailable || globalFullRun {
+		return false
+	}
+
+	for _, changedFile := range changesetFiles {
+		if strings.HasSuffix(testFile, changedFile) {
+			return false
+		}
+		for _, pattern := range getWatchPatterns(testFile) {
+			if pattern.Match(changedFile) {
+				return false
+			}
+		}
+	}
+	return true
+}
+
+func getWatchPatterns(filename string) []glob.Glob {
+	file, err := os.Open(filename)
+	if err != nil {
+		log.Fatal(err.Error())
+	}
+	defer file.Close()
+	scanner := bufio.NewScanner(file)
+
+	watchSection := false
+	patterns := []glob.Glob{}
+	for scanner.Scan() {
+		line := scanner.Text()
+		// check for watch block
+		if strings.HasPrefix(line, "// @watch") {
+			if watchSection {
+				break
+			}
+			watchSection = true
+		}
+		if !watchSection {
+			continue
+		}
+
+		line = strings.TrimPrefix(line, "// ")
+		if line != "" {
+			globPattern, err := glob.Compile(line, '.', '/')
+			if err != nil {
+				log.Fatal("Invalid glob pattern '%s' (skipped): %v", line, err)
+			}
+			patterns = append(patterns, globPattern)
+		}
+	}
+	// if no watch block in file
+	if !watchSection {
+		patterns = append(patterns, glob.MustCompile("*"))
+	}
+	return patterns
+}

tests/e2e/dashboard-ci-status.test.e2e.js

@@ -1,4 +1,9 @@
 // @ts-check
+
+// @watch start
+// web_src/js/components/DashboardRepoList.vue
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, load_logged_in_context} from './utils_e2e.js';
 

tests/e2e/e2e_test.go

@@ -38,6 +38,7 @@ func TestMain(m *testing.M) {
 	defer cancel()
 
 	tests.InitTest(true)
+	initChangedFiles()
 	testE2eWebRoutes = routers.NormalRoutes()
 
 	os.Unsetenv("GIT_AUTHOR_NAME")
@@ -100,6 +101,11 @@ func TestE2e(t *testing.T) {
 		_, filename := filepath.Split(path)
 		testname := filename[:len(filename)-len(filepath.Ext(path))]
 
+		if canSkipTest(path) {
+			fmt.Printf("No related changes for test, skipping: %s\n", filename)
+			continue
+		}
+
 		t.Run(testname, func(t *testing.T) {
 			// Default 2 minute timeout
 			onForgejoRun(t, func(*testing.T, *url.URL) {

tests/e2e/example.test.e2e.js

@@ -1,4 +1,11 @@
 // @ts-check
+
+// @watch start
+// templates/user/auth/**
+// web_src/js/features/user-**
+// modules/{user,auth}/**
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, save_visual} from './utils_e2e.js';
 

tests/e2e/explore.test.e2e.js

@@ -1,6 +1,12 @@
 // @ts-check
 // document is a global in evaluate, so it's safe to ignore here
 // eslint playwright/no-conditional-in-test: 0
+
+// @watch start
+// templates/explore/**
+// web_src/modules/fomantic/**
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test} from './utils_e2e.js';
 

tests/e2e/issue-comment.test.e2e.js

@@ -1,4 +1,11 @@
 // @ts-check
+
+// @watch start
+// web_src/js/features/comp/**
+// web_src/js/features/repo-**
+// templates/repo/issue/view_content/*
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, login} from './utils_e2e.js';
 

tests/e2e/issue-sidebar.test.e2e.js

@@ -1,4 +1,11 @@
 // @ts-check
+
+// @watch start
+// templates/repo/issue/view_content/**
+// web_src/css/repo/issue-**
+// web_src/js/features/repo-issue**
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, login} from './utils_e2e.js';
 

tests/e2e/markdown-editor.test.e2e.js

@@ -1,4 +1,10 @@
 // @ts-check
+
+// @watch start
+// web_src/js/features/comp/ComboMarkdownEditor.js
+// web_src/css/editor/combomarkdowneditor.css
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, load_logged_in_context, login_user} from './utils_e2e.js';
 

tests/e2e/markup.test.e2e.js

@@ -1,4 +1,9 @@
 // @ts-check
+
+// @watch start
+// web_src/css/markup/**
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test} from './utils_e2e.js';
 

tests/e2e/org-settings.test.e2e.js

@@ -1,4 +1,11 @@
 // @ts-check
+
+// @watch start
+// templates/org/team/new.tmpl
+// web_src/css/form.css
+// web_src/js/features/org-team.js
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, login} from './utils_e2e.js';
 import {validate_form} from './shared/forms.js';

tests/e2e/profile_actions.test.e2e.js

@@ -1,4 +1,11 @@
 // @ts-check
+
+// @watch start
+// routers/web/user/**
+// templates/shared/user/**
+// web_src/js/features/common-global.js
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, load_logged_in_context} from './utils_e2e.js';
 

tests/e2e/reaction-selectors.test.e2e.js

@@ -1,4 +1,10 @@
 // @ts-check
+
+// @watch start
+// web_src/js/features/comp/ReactionSelector.js
+// routers/web/repo/issue.go
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, load_logged_in_context} from './utils_e2e.js';
 

tests/e2e/release.test.e2e.js

@@ -1,4 +1,15 @@
 // @ts-check
+
+// @watch start
+// models/repo/attachment.go
+// modules/structs/attachment.go
+// routers/web/repo/**
+// services/attachment/**
+// services/release/**
+// templates/repo/release/**
+// web_src/js/features/repo-release.js
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, save_visual, load_logged_in_context} from './utils_e2e.js';
 import {validate_form} from './shared/forms.js';

tests/e2e/repo-code.test.e2e.js

@@ -1,4 +1,11 @@
 // @ts-check
+
+// @watch start
+// web_src/js/features/repo-code.js
+// web_src/css/repo.css
+// services/gitdiff/**
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, load_logged_in_context} from './utils_e2e.js';
 
@@ -77,10 +84,3 @@ test('Readable diff', async ({page}, workerInfo) => {
     }
   }
 });
-
-test('Commit graph overflow', async ({page}) => {
-  await page.goto('/user2/diff-test/graph');
-  await expect(page.getByRole('button', {name: 'Mono'})).toBeInViewport({ratio: 1});
-  await expect(page.getByRole('button', {name: 'Color'})).toBeInViewport({ratio: 1});
-  await expect(page.locator('.selection.search.dropdown')).toBeInViewport({ratio: 1});
-});

tests/e2e/repo-commitgraph.test.e2e.js

@@ -1,4 +1,11 @@
 // @ts-check
+
+// @watch start
+// templates/repo/graph.tmpl
+// web_src/css/features/gitgraph.css
+// web_src/js/features/repo-graph.js
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, load_logged_in_context} from './utils_e2e.js';
 
@@ -6,6 +13,13 @@ test.beforeAll(async ({browser}, workerInfo) => {
   await login_user(browser, workerInfo, 'user2');
 });
 
+test('Commit graph overflow', async ({page}) => {
+  await page.goto('/user2/diff-test/graph');
+  await expect(page.getByRole('button', {name: 'Mono'})).toBeInViewport({ratio: 1});
+  await expect(page.getByRole('button', {name: 'Color'})).toBeInViewport({ratio: 1});
+  await expect(page.locator('.selection.search.dropdown')).toBeInViewport({ratio: 1});
+});
+
 test('Switch branch', async ({browser}, workerInfo) => {
   const context = await load_logged_in_context(browser, workerInfo, 'user2');
   const page = await context.newPage();

tests/e2e/repo-migrate.test.e2e.js

@@ -1,4 +1,9 @@
 // @ts-check
+
+// @watch start
+// web_src/js/features/repo-migrate.js
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, load_logged_in_context} from './utils_e2e.js';
 

tests/e2e/repo-settings.test.e2e.js

@@ -1,4 +1,13 @@
 // @ts-check
+
+// @watch start
+// templates/webhook/shared-settings.tmpl
+// templates/repo/settings/**
+// web_src/css/{form,repo}.css
+// web_src/css/modules/grid.css
+// web_src/js/features/comp/WebHookEditor.js
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, login} from './utils_e2e.js';
 import {validate_form} from './shared/forms.js';

tests/e2e/repo-wiki.test.e2e.js

@@ -1,4 +1,10 @@
 // @ts-check
+
+// @watch start
+// templates/repo/wiki/**
+// web_src/css/repo**
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test} from './utils_e2e.js';
 

tests/e2e/right-settings-button.test.e2e.js

@@ -1,4 +1,11 @@
 // @ts-check
+
+// @watch start
+// templates/org/**
+// templates/repo/**
+// web_src/js/webcomponents/overflow-menu.js
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, load_logged_in_context} from './utils_e2e.js';
 

tests/e2e/webauthn.test.e2e.js

@@ -2,6 +2,12 @@
 // SPDX-License-Identifier: MIT
 // @ts-check
 
+// @watch start
+// templates/user/auth/**
+// templates/user/settings/**
+// web_src/js/features/user-**
+// @watch end
+
 import {expect} from '@playwright/test';
 import {test, login_user, load_logged_in_context} from './utils_e2e.js';