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

feat(runner): Config option to prevent logging to the console twice #3288

Conversation

is-already-taken
Copy link
Contributor

This might be the case when Karma.run() is called inside the same
process/terminal as the server is running in (e.g. development
servers).

Fixes #2121, #2799

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@is-already-taken
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@is-already-taken is-already-taken force-pushed the feature-option-to-prevent-runner-logging branch from 0142408 to a7579dc Compare March 30, 2019 12:44
@is-already-taken is-already-taken marked this pull request as ready for review March 30, 2019 12:47
@johnjbarton
Copy link
Contributor

If I understand correctly, you want to prevent output by runner and by reporter?

In general I want to avoid adding yet more and more and more options. This makes our code hard to understand and test.

Why not remove the reporter in the config?
Why not remove the logging in the runner altogether?
Why not make the logging in the runner based on logLevel setting?

@is-already-taken
Copy link
Contributor Author

is-already-taken commented Apr 2, 2019

Thank you for your response.

If I understand correctly, you want to prevent output by runner and by reporter?

Correct.

In general I want to avoid adding yet more and more and more options. This makes our code hard to understand and test.

I understand that.

Why not remove the reporter in the config?

If you mean removing it from the server's config then there would be no reporting at all since (to my understanding) the runner is just echoing the output of the reporter. That's not feasible. Please clarify.

Why not remove the logging in the runner altogether?

Can't reliably tell whether this is sarcasm or if I just don't understand. Please clarify.

Why not make the logging in the runner based on logLevel setting?

I like that one. Would it be acceptable if the runner accepts logLevel just like the server and only echoes the reporter results on level config.LOG_DEBUG? This means that logLevel can be applied in two different places: via the server config / switches and via the runner config / switches. In the runner context this would not configure the runners logger but control the process.stdout.write()

I might change the feature this way if you're fine with it.

@johnjbarton
Copy link
Contributor

I don't use the 'runner' part; I don't really know how it works. My suggestion about not logging in the runner is: if the reporter is logging the result why does the runner need to echo that?

To say it another way: was the original feature -- re-log the result in runner -- misdesigned?

@is-already-taken
Copy link
Contributor Author

I think the original idea was, to have the server running in one terminal or even on one host and have the runner connect to that, be it from another terminal or even host. In that case, you want to see the reporter output there. In this context streaming out the reporter results from the socket to the terminal was "good enough". Though I'm not the maintainer, so I'm not 100% sure if that was the idea.

Now, since we're kicking off the runner in same process as the server, they share the same terminal, hence there should be a option to control the runners outputs.

Another approach proposed in the referenced #2121 was to have the runner emit an event. In that case the CLI would have to listen to that event.

@johnjbarton
Copy link
Contributor

Can you take a look at the alternative in #2121?

@is-already-taken is-already-taken force-pushed the feature-option-to-prevent-runner-logging branch from a7579dc to 8ebb5a7 Compare April 10, 2019 04:44
Allow the consumer to decide whether to echo the runner progress or not.

This might be the case when Karma.run() is called inside the same
process/terminal as the server is running in (e.g. development
servers).

Fixes karma-runner#2121, karma-runner#2799
@is-already-taken is-already-taken force-pushed the feature-option-to-prevent-runner-logging branch from 8ebb5a7 to 10b9833 Compare April 10, 2019 18:40
The CLI shall echo the progress, not the runner itself.

Fixes karma-runner#2121, karma-runner#2799
@is-already-taken is-already-taken force-pushed the feature-option-to-prevent-runner-logging branch from 10b9833 to cca8994 Compare April 11, 2019 04:44
@is-already-taken
Copy link
Contributor Author

A friendly review reminder.

@johnjbarton johnjbarton merged commit 62d4c5a into karma-runner:master Jul 30, 2019
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.

Disable or listen to the runner logs
3 participants