Improve Testing Quality of Life (#144)

TylerJang27 · web-flow · commit 6b5b2f8fd117 · 2023-02-08T10:31:27.000-08:00
Lots of small fixes:
1. Alters the tests run on PRs and nightly. Full breakdown in
filters.yaml. Brief summary below.
a. If an individual linter or its subdir is changed, run tests (`[KGV,
Latest]`) on that linter.
b. If a test setup file in `tests/` is changed, run tests (`KGV`) on all
linters.
c. If `.trunk/trunk.yaml` is changed, run tests (`KGV`) on all linters
(important to include trunk version behavior).
d. If `linters/` or `readme.md` or `tests/` are changed, run repo tests.
e. Nightlies will run all tests (`[Snapshots, Latest]`) based on plugin
config in `main` AND all tests (`[Snapshots, Latest]`) on the latest
tagged release (and upload the result)
2. Add `SANDBOX_DEBUG` env var that preserves and logs the sandbox test
dir.
3. Expand the generate script for the helper action to include more
pertinent information about writing/configuring tests.
4. Add some additional information to the `tests/readme.md`.

I intend to eventually remove the logic that makes nightlies run on PRs
that modify .github/workflows/nightly.yaml
diff --git a/.github/filters.yaml b/.github/filters.yaml
@@ -0,0 +1,29 @@
+# Run tests against all linters if the testing framework or trunk version changes.
+# These tests will be run with KnownGoodVersion.
+# e.g. `PLUGINS_TEST_LINTER_VERSION=KnownGoodVersion npm test linters`
+all-linters:
+  - tests/driver/**
+  - tests/reporter/**
+  - tests/types/**
+  - tests/utils/**
+  - tests/index.ts
+  - .trunk/trunk.yaml
+
+# Run tests against specific linters if existing linters have been modified or updated.
+# These test will be run with KnownGoodVersion and Latest to verify linter integrity at the latest and fallback version.
+# Someone should not be able to add a new linter without verifying that it also works on the latest version.
+# e.g. `PLUGINS_TEST_LINTER_VERSION=[KnownGoodVersion,Latest] npm test linters/sqlfluff`
+linters:
+  - linters/**
+
+# Run tests against the health and configuration of the repo if any plugin definition
+# or testing framework change is made.
+# e.g. `npm test tests/repo_tests`
+repo-tests:
+  - tests/**
+  - linters/**
+  - actions/**
+  - readme.md
+  - .trunk/trunk.yaml
+# Nightly jobs will run on all linters with Snapshots and Latest.
+# e.g. `PLUGINS_TEST_LINTER_VERSION=[Snapshots,Latest] npm test linters`
diff --git a/.github/workflows/nightly.yaml b/.github/workflows/nightly.yaml
@@ -10,9 +10,9 @@ on:
 permissions: read-all
 
 jobs:
-  # TODO(Tyler): Forward json outputs from job
-  # Run tests against all linters for snapshots and latest version
-  linter_tests:
+  # Run tests against all linters for snapshots and latest version as they exist on main
+  # This job is used to diagnose plugin config health in advance of a release
+  linter_tests_main:
     name: Plugin Tests
     runs-on: ${{ matrix.os }}
     timeout-minutes: 60
@@ -24,7 +24,75 @@ jobs:
     steps:
       - name: Checkout
         uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0
-        # TODO(Tyler): Iron out ref for tests
+
+      - name: Cache tool downloads
+        uses: actions/cache@627f0f41f6904a5b1efbaed9f96d9eb58e92e920 # v3.2.4
+        with:
+          path: /tmp/plugins_testing_download_cache
+          # No need to key on trunk version unless we change how we store downloads.
+          key: trunk-${{ runner.os }}
+
+      - name: Delete cache
+        # For now, avoid deleting cache on pull request changes to nightly. This improves PR experience.
+        if: env.TRIGGER != 'pull_request'
+        run: |
+          if [ -d "/tmp/plugins_testing_download_cache" ]
+          then
+            tmp_dir=/tmp/${GITHUB_RUN_ID}-${GITHUB_RUN_NUMBER}-${GITHUB_RUN_ATTEMPT}
+            mv "/tmp/plugins_testing_download_cache" ${tmp_dir}
+            chmod -R u+w ${tmp_dir}
+            rm -rf ${tmp_dir}
+          fi
+        shell: bash
+
+      - name: Linter Tests
+        uses: ./.github/actions/linter_tests
+        with:
+          linter-version: ${{ matrix.linter-version }}
+
+  # Run tests against all linters for snapshots and latest version as they exist in latest release
+  # This job is used to update the list of validated versions
+  linter_tests_release:
+    name: Plugin Tests
+    runs-on: ${{ matrix.os }}
+    timeout-minutes: 60
+    strategy:
+      fail-fast: false
+      matrix:
+        linter-version: [Snapshots, Latest]
+        os: [ubuntu-latest, macos-latest]
+    steps:
+      - name: Retrieve git history
+        uses: actions/checkout@v3
+        with:
+          fetch-depth: 0
+
+      # This assumes that any changes on main since the last release are backwards compatible
+      # in terms of allowing existing linter tests to run.
+      - name: Preserve test runner latest behavior
+        run: |
+          cp -r tests tests.bak
+          cp package.json package.json.bak
+          cp package-lock.json package-lock.json.bak
+
+      - name: Get Latest Release
+        id: get-release
+        uses: WyriHaximus/github-action-get-previous-tag@385a2a0b6abf6c2efeb95adfac83d96d6f968e0c # v1.3.0
+        with:
+          # only use releases tagged v<semver>
+          prefix: v
+
+      - name: Checkout
+        uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0
+        with:
+          ref: ${{ steps.get-release.outputs.tag }}
+          clean: false
+
+      - name: Overwrite test runners with latest behavior
+        run: |
+          mv tests.bak tests
+          mv package.json.bak package.json
+          mv package-lock.json.bak package-lock.json
 
       - name: Cache tool downloads
         uses: actions/cache@627f0f41f6904a5b1efbaed9f96d9eb58e92e920 # v3.2.4
@@ -62,7 +130,7 @@ jobs:
 
   upload_test_results:
     name: Upload Test Results
-    needs: linter_tests
+    needs: linter_tests_release
     runs-on: ubuntu-latest
     # Still run on test failure
     if: always()
diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml
@@ -27,10 +27,64 @@ jobs:
       - name: Trunk Check
         uses: trunk-io/trunk-action@9cf65e08e822e9842fd9ef7ed2a2bd9092de0986 # v1.0.6
 
+  detect_changes:
+    name: Detect changed files
+    runs-on: ubuntu-latest
+    timeout-minutes: 5
+
+    outputs:
+      linters: ${{ steps.filter.outputs.linters }}
+      repo-tests: ${{ steps.filter.outputs.repo-tests }}
+      # "linters" if ${{ steps.filter.outputs.all-linters }} is 'true'
+      all-linters: ${{ steps.post-filter.outputs.out }}
+      # shortened paths to linter subdirs
+      linters-files: ${{ steps.post-filter-paths.outputs.out }}
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0
+
+      - name: Determine upstream
+        run: |
+          head_sha=$(git rev-parse HEAD)
+          git -c protocol.version=2 fetch -q \
+            --no-tags \
+            --no-recurse-submodules \
+            --depth=2 \
+            origin "${head_sha}"
+          upstream=$(git rev-parse HEAD^1)
+          echo "TEST_UPSTREAM=${upstream}" >>"${GITHUB_ENV}"
+
+      - name: Detect changed paths
+        uses: dorny/paths-filter@4512585405083f25c027a35db413c2b3b9006d50 # v2.11.1
+        id: filter
+        with:
+          base: ${{ env.TEST_UPSTREAM }}
+          filters: .github/filters.yaml
+          list-files: shell
+
+      - name: Suggest all linter tests
+        id: post-filter
+        if: steps.filter.outputs.all-linters
+        run: echo "out=linters" >> "$GITHUB_OUTPUT"
+
+      - name: Suggest normalized individual linter paths
+        id: post-filter-paths
+        if: steps.filter.outputs.linters
+        run: |
+          echo ${{steps.filter.outputs.linters_files}} |
+          grep -oP "\Klinters/.*?(?=/)" |
+          tr '\n' ' ' |
+          echo "out=$(cat)" >> "$GITHUB_OUTPUT"
+
   # Run tests against all linters for known_good_version and latest version
   linter_tests:
     name: Plugin Tests
     runs-on: ${{ matrix.os }}
+    needs: detect_changes
+    if:
+      needs.detect_changes.outputs.linters == 'true' || needs.detect_changes.outputs.all-linters ==
+      'linters'
     timeout-minutes: 60
     strategy:
       fail-fast: false
@@ -48,11 +102,25 @@ jobs:
           key: trunk-${{ runner.os }}
 
       - name: Linter Tests
+        # Run tests using KnownGoodVersion with any modified linters and conditionally all linters
         uses: ./.github/actions/linter_tests
         with:
           linter-version: KnownGoodVersion
+          append-args:
+            ${{ needs.detect_changes.outputs.all-linters }} ${{
+            needs.detect_changes.outputs.linters-files }}
+
+      - name: Latest Tests Latest
+        # Run tests on Latest with any modified linters (see filters.yaml). Don't run when cancelled.
+        if: ${{ (failure() || success()) && needs.detect_changes.outputs.linters == 'true' }}
+        uses: ./.github/actions/linter_tests
+        with:
+          linter-version: Latest
+          append-args: ${{needs.detect_changes.outputs.linters-files }}
 
   # Run repo healthcheck tests
   repo_tests:
     name: Repo Tests
+    needs: detect_changes
+    if: needs.detect_changes.outputs.repo-tests == 'true'
     uses: ./.github/workflows/repo_tests.reusable.yaml
diff --git a/.github/workflows/repo_tests.reusable.yaml b/.github/workflows/repo_tests.reusable.yaml
@@ -35,7 +35,7 @@ jobs:
 
       - name: Run repo tests
         run: |
-          PATH=$(npm bin):$PATH npm test --ci tests
+          PATH=$(npm bin):$PATH npm test --ci tests/repo_tests
         env:
           PLUGINS_TEST_CLI_VERSION: ${{ inputs.cli-version }}
           PLUGINS_TEST_CLI_PATH: ${{ inputs.cli-path }}
diff --git a/.trunk/trunk.yaml b/.trunk/trunk.yaml
@@ -11,7 +11,7 @@ plugins:
   sources:
     - id: trunk
       uri: https://github.com/trunk-io/plugins
-      ref: 454583aa8bb4d092db343c7c4a71a72a19938768
+      ref: 454583aa8bb4d092db343c7c4a71a72a19938768 # run the plugins repo at its own latest commit
 
 runtimes:
   enabled:
@@ -67,7 +67,7 @@ actions:
       description: Run tests on plugin configuration and documentation
       runtime: node
       packages_file: package.json
-      run: npm test tests/
+      run: npm test tests/repo_tests
       triggers:
         - git_hooks: [pre-push]
 
diff --git a/linters/sqlfluff/sqlfluff.test.ts b/linters/sqlfluff/sqlfluff.test.ts
@@ -4,8 +4,7 @@ import { linterCheckTest, linterFmtTest, TestCallback } from "tests";
 // trunk check linters/sqlfluff/test/basic_check.in.sql --force --filter=sqlfluff --output=json
 linterCheckTest({ linterName: "sqlfluff", namedTestPrefixes: ["basic_check"] });
 
-// Due to sqlfluff's fix subcommand being disabled by default, we need to manually enable it in our test's
-// trunk.yaml.
+// Due to sqlfluff's fix subcommand being disabled by default, we need to manually enable it in the trunk.yaml.
 const fmtCallbacks: TestCallback = (driver) => {
   const trunkYamlPath = ".trunk/trunk.yaml";
   const currentContents = driver.readFile(trunkYamlPath);
diff --git a/security.md b/security.md
@@ -11,6 +11,6 @@ Instead we recommend all users upgrade to the latest release as it comes out.
 
 ## Reporting a Vulnerability
 
-Please reposrt (suspected) security vulnerabilities to security@trunk.io. You will receive a
-response from the team within 48 hours. If the issue is confirmed, we will create a new release as
-soon as possible.
+Please report (suspected) security vulnerabilities to security@trunk.io. You will receive a response
+from the team within 48 hours. If the issue is confirmed, we will create a new release as soon as
+possible.
diff --git a/tests/driver/index.ts b/tests/driver/index.ts
@@ -218,6 +218,17 @@ export class TrunkDriver {
   tearDown() {
     this.debug("Cleaning up %s", this.sandboxPath);
     const trunkCommand = ARGS.cliPath ?? "trunk";
+
+    // Preserve test directory if `SANDBOX_DEBUG` is truthy.
+    if (ARGS.sandboxDebug) {
+      execFileSync(trunkCommand, ["daemon", "shutdown"], {
+        cwd: this.sandboxPath,
+        env: executionEnv(this.getSandbox()),
+      });
+      console.log(`Preserving test dir ${this.getSandbox()} for linter ${this.linter ?? "N/A"}`);
+      return;
+    }
+
     execFileSync(trunkCommand, ["deinit"], {
       cwd: this.sandboxPath,
       env: executionEnv(this.getSandbox()),
diff --git a/tests/readme.md b/tests/readme.md
@@ -63,12 +63,20 @@ npm test ${path_to_linter_subdir}
 Then, verify that the generated snapshot file includes the results you would expect (e.g. an Object
 with several fileIssues, no taskFailures).
 
+For context, the general test execution is as follows:
+
+1. Create a sandbox testing directory by copying a linter's subdirectory and its `test_data`.
+2. Initialize a base .trunk/trunk.yaml in the sandbox with the version in
+   [.trunk/trunk.yaml](../.trunk/trunk.yaml).
+3. Run `trunk check enable <linter>`.
+4. Run `trunk check` or `trunk fmt` on files with the `<name>.in.<extension>` syntax.
+
 ### Linter Versioning
 
 The first time a test runs, it will attempt to run against a linter's `known_good_version`. This
-snapshot is required for CI mirrors the behavior in CI and is used to validate that a linter runs as
-expected across multiple versions. Subsequent test runs will only run against its latest version
-unless otherwise specified (See [Environment Overrides](#environment-overrides)).
+snapshot mirrors the behavior in CI and is used to validate that a linter runs as expected across
+multiple versions. Subsequent test runs will only run against its latest version unless otherwise
+specified (See [Environment Overrides](#environment-overrides)).
 
 If this causes the test to fail when run with the latest version, this is most likely because there
 are discrepancies in the linter output across versions. Rather than running `npm test -- -u`,
@@ -129,10 +137,12 @@ include:
 - `PLUGINS_TEST_UPDATE_SNAPSHOTS` if `true`, tells tests to use an exact match of the linter version
   when checking the output. Only set this if a linter has introduced a output variation with a
   version change.
+- `SANDBOX_DEBUG` if `true`, prevents sandbox test directories from being deleted, and logs their
+  path for additional debugging.
 
 ### CI
 
-PRs will run 5 types of tests across both ubuntu and macOS:
+PRs will run 5 types of tests across both ubuntu and macOS as applicable:
 
 1. Enable and test all linters with their `known_good_version`, if applicable. To replicate this
    behavior, run: `PLUGINS_TEST_LINTER_VERSION=KnownGoodVersion npm test`. If the
@@ -146,9 +156,9 @@ PRs will run 5 types of tests across both ubuntu and macOS:
 
 ### Debugging
 
-Tests normally complete in less than 1 minute. They may take up to 5 minutes or so if the dependency
-cache is empty (linters need to be downloaded and installed to run the linter tests). Subsequent
-runs will not experience this delay.
+Individual tests normally complete in less than 1 minute. They may take up to 5 minutes or so if the
+dependency cache is empty (linters need to be downloaded and installed to run the linter tests).
+Subsequent runs will not experience this delay.
 
 Errors encountered during test runs are reported through the standard `console`, but additional
 debugging is provided using [debug](https://www.npmjs.com/package/debug). The namespace convention
diff --git a/tests/repo_tests/config_check.test.ts b/tests/repo_tests/config_check.test.ts
diff --git a/tests/repo_tests/readme_inclusion.test.ts b/tests/repo_tests/readme_inclusion.test.ts
diff --git a/tests/repo_tests/test_coverage.test.ts b/tests/repo_tests/test_coverage.test.ts
diff --git a/tests/types/index.ts b/tests/types/index.ts
@@ -34,6 +34,8 @@ export interface TestingArguments {
   /** Whether tests should create new snapshot files if snapshots already exist
    * even if a match is found. */
   dumpNewSnapshot: boolean;
+  /** Prevents the deletion of sandbox test dirs. */
+  sandboxDebug: boolean;
 }
 
 /**** Landing state ****/
diff --git a/tests/utils/index.ts b/tests/utils/index.ts
@@ -47,16 +47,24 @@ const normalizePath = (value?: string): string | undefined => {
  * - PLUGINS_TEST_CLI_PATH specifies an alternative path to a trunk binary.
  * - PLUGINS_TEST_LINTER_VERSION specifies a linter version semantic (see `parseLinterVersion`).
  * - PLUGINS_TEST_UPDATE_SNAPSHOTS uses the snapshot from the enabled version of the linter, creating a new snapshot if necessary.
+ * - SANDBOX_DEBUG prevents test directories from being deleted.
  */
 export const ARGS: TestingArguments = {
   cliVersion: coalesceString(process.env.PLUGINS_TEST_CLI_VERSION),
   cliPath: normalizePath(process.env.PLUGINS_TEST_CLI_PATH),
   linterVersion: parseLinterVersion(process.env.PLUGINS_TEST_LINTER_VERSION ?? ""),
   dumpNewSnapshot: Boolean(process.env.PLUGINS_TEST_UPDATE_SNAPSHOTS),
+  sandboxDebug: Boolean(process.env.SANDBOX_DEBUG),
 };
 // TODO(Tyler): PLUGINS_TEST_LINTER_VERSION is a string version, we should mandate that a test filter is applied
 // to avoid accidental enables.
-if (ARGS.cliVersion || ARGS.cliPath || ARGS.linterVersion || ARGS.dumpNewSnapshot) {
+if (
+  ARGS.cliVersion ||
+  ARGS.cliPath ||
+  ARGS.linterVersion ||
+  ARGS.dumpNewSnapshot ||
+  ARGS.sandboxDebug
+) {
   Debug("Tests").extend("Global")("%o", ARGS);
 }
 
diff --git a/tools/linter-test-helper/generate b/tools/linter-test-helper/generate
@@ -19,14 +19,20 @@ lint:
 """
 
 INITIAL_TEST_CONTENTS = """import { linterCheckTest, linterFmtTest } from "tests";
-// Use if your tool is a linter
+// Uncomment and use if your tool is a linter
 // linterCheckTest({ linterName: "*{}*" });
 
-// Use if your tool is a formatter
+// Uncomment and use if your tool is a formatter
 // linterFmtTest({ linterName: "*{}*" });
 
+// Guidelines for configuring tests:
+//  - By default, linters and formatters will only run on files with syntax `<name>.in.<extension>`
+//  - You can customize test setup using the `preCheck` callback (see git_diff_check.test.ts and golangci_lint.test.ts)
+//  - You can specify additional customization using the `customLinterCheckTest and customLinterFmtTest` helpers
+//  - Additional information on test setup can be found in tests/readme.md
+//
 // If you are unable to write a test for this linter, please document why in your PR, and add
-// it to the list in tests/test_coverage_test.ts
+// it to the list in tests/repo_tests/test_coverage_test.ts
 """
 
 

Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,8 @@ export interface TestingArguments {`
`34`	`34`	`/** Whether tests should create new snapshot files if snapshots already exist`
`35`	`35`	`* even if a match is found. */`
`36`	`36`	`dumpNewSnapshot: boolean;`
	`37`	`+ /** Prevents the deletion of sandbox test dirs. */`
	`38`	`+ sandboxDebug: boolean;`
`37`	`39`	`}`
`38`	`40`
`39`	`41`	`/** Landing state **/`