Skip to content

Conversation

@kentonv
Copy link
Contributor

@kentonv kentonv commented Mar 13, 2025

Description

As originally proposed here: #7672

If stdin is not a TTY, we are probably running in a non-interactive context, and thus "watch" mode is likely to hang forever as the user is not around to press 'q' to quit.

This can come up in particular when using tools or AI agents that want to run the tests using npm run test, but where the "test" script has been define as simply vitest. Of course, an alternative solution would be to change the script to vitest run, but this makes the experience worse for humans running npm run test.

I was unable to figure out how to test this change. In the PR review, it was suggested that isTTY could be overridden in a test fixture, as done in test/watch/fixtures/single-failed/vitest.config.ts. This does not actually appear to work:

  • The vite.config.ts file appears to run in a different process than the CLI under test, so the assignment has no effect.
  • Indeed, when I tried removing the lines from the single-failed fixture, or even overriding isTTY to false instead, this did not break the test reporter-failed.test.ts which uses this fixture. So it appears this existing fixture is not a good example as it is not actually doing what it looks like.

Note: I originally submitted a PR generated by Claude Code. It worked, but @sheremet-va pointed out (below) that there was a cleaner solution. To be fair had I tried to do it by hand from the start I am not sure I would have done any better than Claude, since I'm unfamiliar with this codebase. Oh well! Original Claude Code transcript here: https://claude-workerd-transcript.pages.dev/vite-default-run-when-not-tty

Note 2: I also tried to use Claude Code to try out a bunch of approaches to testing this change that ultimately did not work. The morbidly curious can see the transcript here: https://claude-workerd-transcript.pages.dev/vite-default-run-when-not-tty2

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

@netlify
Copy link

netlify bot commented Mar 13, 2025

Deploy Preview for vitest-dev ready!

Name Link
🔨 Latest commit 7232d95
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/6800ce2032d487000886f856
😎 Deploy Preview https://deploy-preview-7673--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kentonv
Copy link
Contributor Author

kentonv commented Mar 13, 2025

Test failure is presumably unrelated as it's a browser failure and I only changed the CLI code?

@kentonv
Copy link
Contributor Author

kentonv commented Mar 13, 2025

On a side note, this is actually my first time using AI to contribute to someone else's code and I have to say, I feel like I'm cheating or violating some etiquette or something. Apologies if anyone finds this distasteful. I would have felt that way myself, a month ago.

But this meant I could contribute the change without having to learn the codebase, and the change works and looks reasonable? At least I doubt I'd have done better if I spent the time to figure it out myself. Happy to apply any suggestions about how to do it better.

@sheremet-va
Copy link
Member

sheremet-va commented Mar 14, 2025

Test failure is presumably unrelated as it's a browser failure and I only changed the CLI code?

It's just flaky

On a side note, this is actually my first time using AI to contribute to someone else's code and I have to say, I feel like I'm cheating or violating some etiquette or something. Apologies if anyone finds this distasteful. I would have felt that way myself, a month ago.

But this meant I could contribute the change without having to learn the codebase, and the change works and looks reasonable? At least I doubt I'd have done better if I spent the time to figure it out myself. Happy to apply any suggestions about how to do it better.

AI was wrong, the code should be placed here:

Claude was not able to find a good place to test this, since simulating a TTY for tests is awkward. However, if anyone has a suggestion for how this could be tested I would be happy to (prompt Claude to) add one!

There are a few tests where we manually set it, like here

@kentonv
Copy link
Contributor Author

kentonv commented Mar 14, 2025

@sheremet-va Updated to use the much cleaner approach. Sorry about that.

Unfortunately the test strategy you suggested doesn't seem to work -- in fact the existing test fixture you pointed out doesn't seem to be doing what it thinks it is doing. I can delete the line or change the value to false and it doesn't seem to break any tests. It seems like the config file is not evaluated in the same process as the test body, and indeed runVitestCli() runs further subprocesses, so assigning isTTY directly doesn't actually work.

@sheremet-va
Copy link
Member

@sheremet-va Updated to use the much cleaner approach. Sorry about that.

Unfortunately the test strategy you suggested doesn't seem to work -- in fact the existing test fixture you pointed out doesn't seem to be doing what it thinks it is doing. I can delete the line or change the value to false and it doesn't seem to break any tests. It seems like the config file is not evaluated in the same process as the test body, and indeed runVitestCli() runs further subprocesses, so assigning isTTY directly doesn't actually work.

There are other configs that do the same thing. This one is probably just copy pasted.

Copy link
Member

@sheremet-va sheremet-va left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation needs to be updated

As originally proposed here: vitest-dev#7672

If stdin is not a TTY, we are probably running in a non-interactive context, and thus "watch" mode is likely to hang forever as the user is not around to press 'q' to quit.

This can come up in particular when using tools or AI agents that want to run the tests using `npm run test`, but where the "test" script has been define as simply `vitest`. Of course, an alternative solution would be to change the script to `vitest run`, but this makes the experience worse for humans running `npm run test`.

I was unable to figure out how to test this change. In the PR review, it was suggested that `isTTY` could be overridden in a test fixture, as done in `test/watch/fixtures/single-failed/vitest.config.ts`. This does not actually appear to work:
- The `vite.config.ts` file appears to run in a different process than the CLI under test, so the assignment has no effect.
- Indeed, when I tried removing the lines from the `single-failed` fixture, or even overriding isTTY to false instead, this did not break the test `reporter-failed.test.ts` which uses this fixture. So it appears this existing fixture is not a good example as it is not actually doing what it looks like.
@kentonv
Copy link
Contributor Author

kentonv commented Mar 17, 2025

Documentation needs to be updated

Sorry, for some reason I didn't expect the docs to be quite so thorough. Should have checked. Updated now.

There are other configs that do the same thing. This one is probably just copy pasted.

There are several tests which pass isTTY: true into runVitest(), but this seems to be too low-level, as runVitest() bypasses the interpretation of command-line args, which is what we'd need to test here. AFAICT there are no configs which override isTTY while using runVitestCli().

I could try to test the defaults module directly but even this seems hard since it'll only be evaluated on the first import. So I'd have to use a dynamic import trick. And I'd also need a way to override isCI so that the test can function in CI, but this seems non-trivial as well. I haven't found an existing test for the isCI behavior.

Copy link
Contributor

@hi-ogawa hi-ogawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the test is easy or much meaning for this case. I don't mind merging this with just one liner. I just quickly confirmed the behavior by pnpm -C examples/basic test < /dev/null.

Btw, this is just requested by other users of AI agents #7818.

@hi-ogawa hi-ogawa moved this to P2 - 2 in Team Board Apr 11, 2025
@hi-ogawa hi-ogawa added the p2-to-be-discussed Enhancement under consideration (priority) label Apr 11, 2025
@sheremet-va sheremet-va merged commit 6358f21 into vitest-dev:main Apr 17, 2025
12 of 13 checks passed
schogges pushed a commit to kumahq/kuma-gui that referenced this pull request Apr 23, 2025
v3.1.2
   🐞 Bug Fixes
Add global chai variable in vitest/globals (fix: #7474)  -  by @​Jay-Karia in vitest-dev/vitest#7771 and vitest-dev/vitest#7474 (d9297)
Prevent modifying test.exclude when same object passed in coverage.exclude  -  by @​AriPerkkio in vitest-dev/vitest#7774 (c3751)
Fix already hoisted mock  -  by @​hi-ogawa in vitest-dev/vitest#7815 (773b1)
Fix test.scoped inheritance  -  by @​hi-ogawa in vitest-dev/vitest#7814 (db6c3)
Remove pointer-events-none after resizing the left panel  -  by @​alexprudhomme in vitest-dev/vitest#7811 (a7e77)
Default to run mode when stdin is not a TTY  -  by @​kentonv, @​hi-ogawa and @​sheremet-va in vitest-dev/vitest#7673 (6358f)
Use happy-dom/jsdom types for envionmentOptions  -  by @​hi-ogawa in vitest-dev/vitest#7795 (67430)
browser:
Fix transform error before browser server initialization  -  by @​hi-ogawa in vitest-dev/vitest#7783 (5f762)
Fix mocking from outside of root  -  by @​hi-ogawa in vitest-dev/vitest#7789 (03f55)
Scale iframe for non ui case  -  by @​hi-ogawa in vitest-dev/vitest#6512 (c3374)
coverage:
await profiler calls  -  by @​AriPerkkio in vitest-dev/vitest#7763 (795a6)
Expose profiling timers  -  by @​AriPerkkio in vitest-dev/vitest#7820 (5652b)
deps:
Update all non-major dependencies  -  in vitest-dev/vitest#7765 (7c3df)
Update all non-major dependencies  -  in vitest-dev/vitest#7831 (15701)
runner:
Correctly call test hooks and teardown functions  -  by @​sheremet-va in vitest-dev/vitest#7775 (3c00c)
Show stacktrace on test timeout error  -  by @​hi-ogawa in vitest-dev/vitest#7799 (df33b)
ui:
Load panel sizes from storage on initial load  -  by @​userquin in vitest-dev/vitest#7265 (6555d)
vite-node:
Named export should overwrite export all  -  by @​hi-ogawa in vitest-dev/vitest#7846 (5ba0d)
Add ERR_MODULE_NOT_FOUND code error if module cannot be loaded  -  by @​sheremet-va in vitest-dev/vitest#7776 (f9eac)
   🏎 Performance
browser: Improve browser parallelisation  -  by @​sheremet-va in vitest-dev/vitest#7665 (816a5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p2-to-be-discussed Enhancement under consideration (priority)

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants