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

Add clearTerminal setting #1014

Merged
merged 9 commits into from
Jul 2, 2023
Merged

Conversation

jgillick
Copy link
Contributor

When auto-run is disabled, it's convenient to have the terminal clear and only show the results from the most recent run.

In this PR:

  • Added a clearTerminal boolean setting.
  • Added clear() to ExtOutputTerminal
  • Clears the terminal when a test run is scheduled if the setting is set to true.

Copy link
Collaborator

@connectdotz connectdotz left a comment

Choose a reason for hiding this comment

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

@jgillick Thanks for the PR, I think this is a useful feature. Sorry about the delayed response.

A few comments:

  1. let's change the setting to "autoClearTerminal".
  2. This probably only worked for the interactive runs, which is fine. But if we can find a way to make it also work for "watch" mode, that will be even better. The "watch" mode doesn't go through the "schedule" call as it is initiated outside of vscode. But it did emit other signals... see the process-isteners code for clues.

@jgillick
Copy link
Contributor Author

Thanks for the feedback.

  • I've updated the setting name to autoClearTerminal
  • It will now also auto-clear when in watch mode. (basically anytime it logs > Test run started)

@coveralls
Copy link

coveralls commented Jun 10, 2023

Pull Request Test Coverage Report for Build 5381990409

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 97.757%

Totals Coverage Status
Change from base Build 5370976313: -0.01%
Covered Lines: 3548
Relevant Lines: 3568

💛 - Coveralls

Copy link
Collaborator

@connectdotz connectdotz left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the update. The code's looking good, but we need to improve the test coverage for the new changes. After all, this is a test extension, right? We gotta walk the talk! 😄

When you get a chance, could you also update the Customization section in the README.md?

Apologies for the slow response on my end. This is a super useful change, and once those updates are made, I'll be ready to merge it in. Thanks again!

@jgillick
Copy link
Contributor Author

@connectdotz thanks for the feedback. I've added unit tests and updated the README.

Copy link
Collaborator

@connectdotz connectdotz left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Looks like there are some merge issues. Please see the comment.

BTW, you can yarn run ci before check-in to ensure the tests and formats are correct before check-in.

@@ -111,6 +111,7 @@ export const getExtensionResourceSettings = (

return {
jestCommandLine: getSetting<string>('jestCommandLine'),
autoClearTerminal: config.get<boolean>('autoClearTerminal') ?? false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use getSetting<boolean> instead

@jgillick
Copy link
Contributor Author

Whoops, sorry about that. It should be resolved now.

Copy link
Collaborator

@connectdotz connectdotz left a comment

Choose a reason for hiding this comment

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

It seems that maybe a test is accidentally deleted?

tests/test-provider/test-item-data.test.ts Outdated Show resolved Hide resolved
@connectdotz connectdotz merged commit 734ef2b into jest-community:master Jul 2, 2023
@connectdotz
Copy link
Collaborator

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants