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

Testing console output #10

Closed
iiroj opened this issue Apr 24, 2020 · 11 comments
Closed

Testing console output #10

iiroj opened this issue Apr 24, 2020 · 11 comments

Comments

@iiroj
Copy link

iiroj commented Apr 24, 2020

Hello,

we're in the process of migrating lint-staged to Listr2, but I have some question regarding test output assertion.

The orginal Listr's verbose renderer printed [started] and [completed] strings, while Listr2 uses fancy symbols like and . These symbols are different on each platform, though, so testing in a CI environment is a bit difficult.

You can see the progress in the PR lint-staged/lint-staged#852 and how the assertions fail on our Windows tests: https://ci.appveyor.com/project/okonet/lint-staged/builds/32413566/job/qyae7t039cckykre

@cenk1cenk2
Copy link
Collaborator

That is a easy fix I will add the test renderer as default.

@cenk1cenk2
Copy link
Collaborator

image
Something in the ballpark of this hopefully does it. I will also add tests here when I have free time but been lazy lately.

To call the test renderer set the renderer to test.

  const tasks = new Listr<ListrCtx>([
    {
      // You can nest Listr objects, as in the original but now you can also call a new listr class throught task.newListr and inject a type as well
      title: 'Concurrent sub test.',
      task: (ctx, task): Listr => task.newListr<ListrCtx>([
        ...someTasks
  ], {
    renderer:'test'
  })

I will merge after I perform usual manual tests.

@iiroj
Copy link
Author

iiroj commented Apr 24, 2020

If it’s configurable I can also set renderer: ’test’ during setup.

EDIT: thanks!

@cenk1cenk2
Copy link
Collaborator

Added a test renderer with #11, published with version 1.3.8.

Take a look at Logger and Test Renderer for expected outputs on the given occasion. But they are all text-based now. I will also add automated tests hopefully soon.

As mentioned you can pass in test renderer with { renderer: 'test' } as an option to the given task.

@cenk1cenk2
Copy link
Collaborator

cenk1cenk2 commented Apr 24, 2020

For any more issues, I kindly ask you to please open up a new issue, hopefully I was helpful! Have a nice day. I will be watching the thread.

@iiroj
Copy link
Author

iiroj commented Apr 24, 2020

Thanks, seems to be just what we need! I'll also use the test renderer when TERM=dumb just in case those don't support the fancy glyphs.

@cenk1cenk2
Copy link
Collaborator

What you can also do is to set fallback renderer to test instead of the verbose by passing in { nonTTYRenderer: 'test' } since it should automatically detect non-tty environment.

@cenk1cenk2
Copy link
Collaborator

@iiroj I kindly wanted to inform you about the upcoming changes beforehand.

For further updates with Listr2 there had to be breaking changes how the renderer options get passed in, I just wanted to go ahead and kindly inform you about the breaking changes that were discussed on the original repository by SamVerschueren in Originally posted by @SamVerschueren in SamVerschueren/listr#143 (comment).

This will affect how the renderer and task-scoped renderer options get passed in. This can be seen in detail in here.

While doing that I also exchanged the 'test' renderer which you are mainly using back to 'verbose' since verbose renderer does not use icons anymore and they are basically the same now. So this will not require a major refactoring and a ctrl+h would be more than enough.

Some of the issues about the collection of errors which can be seen here and cli-truncate functionality has been fixed that you mentioned here in this pull request.

@iiroj
Copy link
Author

iiroj commented May 6, 2020

@cenk1cenk2 thanks for the info! I'll see about migrating to it. Since I already moved the error collecting out of the old behaviour of listr before this, does this affect lint-staged? It was a good change to do in lint-staged IMO since it streamlined the collection of task output (other than errors).

I assume I will have to at least change the errors key to something else in the context. Right now it's a Set and various error cases push symbols to it, so that it's easy to check if (errors.has(SomeSymbol))

@iiroj
Copy link
Author

iiroj commented May 6, 2020

Sorry to ask an otherwise unrelated question, but does the inclusion of cli-truncate mean that the task name will always be on a single line? Lint-staged currently shortens function task names until the first space in the command string [Function] eslint ..., but this might allow us to skip the truncating and just pass the entire string as the name. This can be really long, since it potentially includes full (absolute) file paths of all staged files during a git commit.

@cenk1cenk2
Copy link
Collaborator

I think it should not cause any problems for the errors because error collection is done out of the context. So after the task has been run, you shoud access the errors via task.err. As a result if this way works much better for you, I think no refactoring is needed.

const task = new Listr(...)
const ctx = await task.run()
logger.fail(task.err)
// will show all of the errors that are encountered through execution

If your cli-truncate action relies on process.stdout.columns as well it will more or less do the same basic thing utilizing the plugin cli-truncate.

Only thing that may need refactoring is the rendererOptions. They now have their own key and not set from the root of the options and they will change their autocomplete functionallity based on which renderer is selected. This can be seen here in detail.

I have also removed test renderer which you are using for the tests since I equalize verbose and test renderer in a way that verbose renderer now has their icon output as optionals. So since they are basically the same thing now and verbose seemed like a better naming scheme then test, I removed test renderer. But this should be a matter of find&replace { renderer: 'test' } to { renderer: 'verbose' }. If you so wish you can pass a mock funtion for testing purposes to verbose renderer extending Logger class { renderer: 'verbose', rendererOptions: { logger: MyLoggerClass } }, which I think should make testing more easier. But it can also work as a plug-and-play replacement, just changing the logger name.

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

No branches or pull requests

2 participants