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

Please Add The Ability To Define Custom Test Trigger Patterns #6457

Open
4 tasks done
klondikemarlen opened this issue Sep 6, 2024 · 9 comments
Open
4 tasks done
Labels
p2-nice-to-have Not breaking anything but nice to have (priority) pr welcome

Comments

@klondikemarlen
Copy link

klondikemarlen commented Sep 6, 2024

Clear and concise description of the problem

As a developer using Vitest I want the ability to have an .html or .txt file trigger the re-run of a specific test file so that I can have an easier time testing mailers and email templates.

I'd like to see an option similar to what Ruby's Guard has.
In Guard it would look something like this:

guard :minitest, cli: '--force' do
  # Watch for changes in .ts, .html, or .txt files in mailers or templates directories
  watch(%r{^src/(mailers|templates)/(.*)\.(ts|html|txt)$}) do |m|
    "api/tests/mailers/#{m[2]}.test.ts"
  end
end

This would cause changes to:

  • api/src/templates/workflow-steps/next-step-mailer.html
  • api/src/templates/workflow-steps/next-step-mailer.txt
  • api/src/mailers/workflow-steps/next-step-mailer.ts
    to all trigger the test api/tests/mailers/workflow-steps/next-step-mailer.test.ts to run.

Suggested solution

Given that the JS equivalent might be something like

const chokidar = require('chokidar');
const { exec } = require('child_process');

// Watch for changes in .ts, .html, or .txt files in mailers or templates directories
chokidar.watch('src/{mailers,templates}/**/*.{ts,html,txt}')
  .on('change', (path) => {
    const match = path.match(/^src\/(mailers|templates)\/(.*)\.(ts|html|txt)$/);
    if (match) {
      const testFile = `api/tests/mailers/${match[2]}.test.ts`;
      console.log(`Detected change in: ${path}. Running ${testFile}`);
      exec(`npx vitest run ${testFile}`, (err, stdout, stderr) => {
        if (err) {
          console.error(`Error: ${stderr}`);
          return;
        }
        console.log(stdout);
      });
    }
  });

It would probably be possible to make a vitest config option test.watch.triggerPatterns with an interface like:

import { defineConfig } from "vitest/config";

export default defineConfig({
  test: {
    watch: {
      triggerPatterns: [{
        pattern: /^src\/(mailers|templates)\/(.*)\.(ts|html|txt)$/,
        testToRun: (match) => {
          return `api/tests/mailers/${match[2]}.test.ts`
        },
      }],
    },
  },
});

Alternative

Option 1

Use https://vitest.dev/config/#forcereruntriggers

import { defineConfig } from "vitest/config";

export default defineConfig({
  test: {
    forceRerunTriggers: [
      "**/*.(html|txt)", // Rerun tests when data files change
    ],
  },
});

While I'm happy that this option exists; I don't love it, because it forces all the tests to re-run, not just the relevant one.

Option 2

Use direct import at top of test file
e.g.

// in api/tests/mailers/workflow-steps/next-step-mailer.test.ts
import "@/templates/workflow-steps/next-step-mailer.html?raw"
import "@/templates/workflow-steps/next-step-mailer.txt?raw"

I can't decide if I like this option more or less than the previous option.
On one hand, its efficient, and pretty straight forward.
On the other hand its not a global configuration, so I'm effectively leaking config into individual files.
Also its likely that future developers will have no idea what this does without an explanatory comment.

Additional context

Guard docs at https://github.com/guard/guard/wiki/Custom-matchers-in-watches#using-custom-matchers-in-watches
Guard DSL source https://github.com/guard/guard/blob/7ccce79c46a6e9d1acaca989bc1a2b72d3806221/lib/guard/dsl.rb#L209
Guard Watcher implementation source https://github.com/guard/guard/blob/7ccce79c46a6e9d1acaca989bc1a2b72d3806221/lib/guard/watcher.rb#L13

Validations

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Sep 7, 2024

I think this is mostly possible with a vite plugin using addWatchFile (or also injecting ?raw import from transform would also work). I made a simple demo here:

https://stackblitz.com/edit/vitest-dev-vitest-qq31nt?file=vite.config.ts

function watchReRunDepPlugin(): Plugin {
  return {
    name: 'watch-rerun-dep-plugin',
    transform(code, id, options) {
      // matching a test file such as
      //   - test/xxx.test.ts
      // then call `this.addWatchFile` for
      //   - src/xxx.txt
      //   - src/xxx.html
      const match = id.match(/\/(\w*)\.test\.ts$/);
      if (match) {
        const name = match[1]!;
        for (const ext of ['.txt', '.html']) {
          const file = path.resolve(id, '../../src', name + ext);
          if (fs.existsSync(file)) {
            this.addWatchFile(file);
          }
        }
      }
    },
  };
}

Slight difference is that the pattern is reversed from what you suggested since it needs to list "dependencies" for a given "test file". If that part is abstracted in some ways, then the plugin itself is really a tiny wrapper of addWatchFile, for example:

function watchReRunDepPlugin({
  getDeps,
}: {
  getDeps: (file: string) => string[];
}): Plugin {
  return {
    name: "watch-rerun-dep-plugin",
    transform(code, id, options) {
      for (const dep of getDeps(id)) {
        this.addWatchFile(dep);
      }
    },
  };
}

@sheremet-va sheremet-va added the p2-to-be-discussed Enhancement under consideration (priority) label Sep 7, 2024
@klondikemarlen
Copy link
Author

That looks perfect! For other people, like me, who have never heard of addWatchFile, its documentation can be found at https://rollupjs.org/plugin-development/#this-addwatchfile.
Vite uses Rollup, as interpreted from Vite docs comment "... which are based on Rollup's well-designed plugin interface .."

Maybe I'll investigate writing a plugin to clone the watch feature from Ruby Guard.

@klondikemarlen
Copy link
Author

I've made it as far as creating a repository and npm package stub, and I'll try to pull in the code written by @hi-ogawa. I know almost nothing about publishing npm packages so apologies if it's a mess.

klondikemarlen added a commit to klondikemarlen/vite-plugin-guard-like-watch that referenced this issue Sep 9, 2024
@klondikemarlen
Copy link
Author

I wrote a basic plugin concept at https://github.com/klondikemarlen/vite-plugin-guard-like-watch and https://www.npmjs.com/package/vite-plugin-guard-like-watch.

I need to test out what happens when I import this package into another project; and probably set up a build system so that it only includes the complied version or something.

@klondikemarlen
Copy link
Author

I have no idea how the build system works, but I tested the built version in a secont project and it worked. So I'm going 1.0 for https://www.npmjs.com/package/vite-plugin-guard-like-watch, and moving on.

Final usage instructions looks like

npm install --save-dev vite-plugin-guard-like-watch

Example vite.config.ts

/// <reference types="vitest/config" />

// Configure Vitest (https://vitest.dev/config/)
import { defineConfig } from "vite"
import guardLikeWatch from "vite-plugin-guard-like-watch"

export default defineConfig({
  test: {
    /* for example, use global to avoid globals imports (describe, test, expect): */
    // globals: true,
  },
  plugins: [
    // Note: debug is optional, but you'll probably need it to get set up.
    guardLikeWatch(/(.*\/example)\.ts/, (match) => [`${match[1]}.html`, `${match[1]}.txt`], true),
    guardLikeWatch({
      pattern: /(.*)\/example\/example\.ts/,
      action: (match) => [`${match[1]}/src/vite-plugin-guard-like-watch.ts`],
      debug: true,
    }),
    // Relative paths will also work, though I'm not entirely sure of the implications. e.g.
    guardLikeWatch(
      /tests\/mailers\/(.*)\.test\.ts/,
      (match) => [`src/templates/${match[1]}.html`, `src/templates/${match[1]}.txt`],
      true
    ),
    guardLikeWatch({
      pattern: /tests\/mailers\/(.*)\.test\.ts/,
      action: (match) => [`src/templates/${match[1]}.html`, `src/templates/${match[1]}.txt`],
      debug: true,
    }),
    // You can also use a string instead of a RegExp. e.g.
    guardLikeWatch(
      "tests/mailers/(.*).test.ts",
      (match) => [`src/templates/${match[1]}.html`, `src/templates/${match[1]}.txt`],
      true
    ),
    guardLikeWatch({
      pattern: "tests/mailers/(.*).test.ts",
      action: (match) => [`src/templates/${match[1]}.html`, `src/templates/${match[1]}.txt`],
      debug: true,
    }),
  ],
})

@sheremet-va sheremet-va added p2-nice-to-have Not breaking anything but nice to have (priority) and removed p2-to-be-discussed Enhancement under consideration (priority) enhancement: pending triage labels Sep 12, 2024
@sheremet-va
Copy link
Member

sheremet-va commented Sep 26, 2024

We should implement it as a config option similar to your proposal here:

import { defineConfig } from "vitest/config";

export default defineConfig({
  test: {
    watchTriggerPatterns: [
      {
        pattern: /^src\/(mailers|templates)\/(.*)\.(ts|html|txt)$/,
        testToRun: (match) => {
          return `api/tests/mailers/${match[2]}.test.ts`
        },
      }
    ],
  },
});

We can just test the regexp when the watcher triggers it here:

onChange = (id: string) => {

@aleclarson
Copy link
Contributor

aleclarson commented Oct 21, 2024

I think it'd be better to have a test be able to specify its file dependencies.

test('foo', () => {
  const foo = fs.readFileSync('foo.html')
  expect.watch('foo.html')
})

I have no opinion on the exact naming or implementation.

edit: Oh I see this was proposed by #3378. What is the rationale for not allowing per-test file dependencies?

@klondikemarlen
Copy link
Author

klondikemarlen commented Oct 21, 2024

@aleclarson The feature you are talking about already exists: if you use lazy imports like const foo = () => import("some/dependency") (or something like that, I forget the syntax) at the top of a test file, editing the dependency will trigger a test re-run.

The dynamic imports on a per-file basis is great on a small scale, for one-off things, but on a larger scale, you end up polluting your tests with test configuration code. And the pattern listed here is a clean and centralized solution that this.

What is the rationale for not allowing per-test file dependencies?

Oops, I race your question; I'm not sure the answer to this, but I'm guessing it's an architecture conflict with the way Rollup generates dependencies?

@hi-ogawa
Copy link
Contributor

I think it'd be better to have a test be able to specify its file dependencies.

@aleclarson Like @klondikemarlen mentioned in alternative 2, we recommend import "xxx?raw" for "per test file" re-run. If you are talking about "per test case" re-run, we don't have such mechanism and it's probably out of scope for now.

import "./foo.html?raw"; // this

test('foo', () => {
  await import("./foo.html?raw") // or this should also work, but still "per-test-file" re-run
  const foo = fs.readFileSync('foo.html')
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority) pr welcome
Projects
Status: Approved
Development

No branches or pull requests

4 participants