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

cli: Hide Watch Usage output when running on non-interactive envs #4958

Merged
merged 8 commits into from
Dec 7, 2017

Conversation

taylon
Copy link
Contributor

@taylon taylon commented Nov 25, 2017

Summary

Watch Usage options are useless in non-interactive environments (like Docker). Currently jest-cli is smart enough to know that it should not clear the terminal in those environments, with this PR it will also not output Watch Usage related stuff either, making the UI cleaner.

Also, because the terminal is not cleared after a test run, the subsequent runs will not print a new line after the "Press w to show more" text, which makes it hard to read and a really bad UX once you have watch running for a couple of test runs.

I'm not familiar with the code base at all, so let me know if there is anything that should be done differently and I'll fix it =D

Test plan

Output in interactive environments is not affected.
Here is how the output looks in Docker before and after this PR:

Before:
before

After:
after

@SimenB SimenB requested a review from rogeliog November 26, 2017 13:02
@@ -0,0 +1,3 @@
import isCI from 'is-ci';

export default process.stdout.isTTY && !isCI;
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I think we could name this file is_interactive so that the import looks like 'import isInteractive from './is_interactive'...

I could even be nice if this was in something like jest-util and then we could use it in other places of the codebase like

packages/jest-cli/src/pre_run_message.js
packages/jest-cli/src/reporters/__tests__/default_reporter.test.js
packages/jest-cli/src/reporters/coverage_reporter.js
packages/jest-cli/src/reporters/default_reporter.js
packages/jest-cli/src/watch.js
packages/jest-util/src/clear_line.js

@rogeliog
Copy link
Contributor

Looks good!

One small comment if you are running jest in watch mode within Docker. You could pass the -it flags and then it would make it interactive docker run -it my-image yarn test

@cpojer
Copy link
Member

cpojer commented Nov 27, 2017

Thanks for sending a PR. Can you make sure "yarn test" passes? :)

@taylon
Copy link
Contributor Author

taylon commented Nov 27, 2017

I renamed and moved isInteractive to jest-util and also replaced usage of process.stdout.isTTY && !isCI with it.

I couldn't figure out why tests are failing in circleci though. Locally I tried yarn test, yarn test-ci and yarn test-ci-partial and everything passes. Do you guys have any idea of what could be wrong?

@SimenB
Copy link
Member

SimenB commented Nov 27, 2017

Do they pass when you do CI=true yarn test? Or somehow emulate the terminal on CI

@taylon
Copy link
Contributor Author

taylon commented Nov 27, 2017

Thanks @SimenB that was exactly it =D

Tests are passing now, I also added tests to is_interactive.
CircleCI is failing because of a yarn error and I can't trigger another build manually, that should be passing though.

@SimenB
Copy link
Member

SimenB commented Nov 27, 2017

I triggered a new CI run.

Can you update the changelog?

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants