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

Expose `log_cli` as a CLI parser option. #3190

Merged
merged 3 commits into from Feb 8, 2018

Conversation

Projects
None yet
5 participants
@s0undt3ch
Contributor

s0undt3ch commented Feb 5, 2018

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of removal, feature, bugfix, vendor, doc or trivial
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
  • Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS, in alphabetical order;
@s0undt3ch

This comment has been minimized.

Contributor

s0undt3ch commented Feb 5, 2018

Is it worth the hassle to automatically enable logs streaming if --log-cli-level is passed, and don't automatically enable it if log_cli_level is set?

@coveralls

This comment has been minimized.

coveralls commented Feb 5, 2018

Coverage Status

Coverage increased (+0.002%) to 92.635% when pulling a4cbd03 on s0undt3ch:feature/logs-stream into ce0a9aa on pytest-dev:features.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Feb 5, 2018

are you aware that py.test -o 'log_cli=true' should already do that
we introduced the power to pass options that way exactly because we wanted to acoid growing the number of cli options as we grow the feature set

@s0undt3ch

This comment has been minimized.

Contributor

s0undt3ch commented Feb 5, 2018

Yes, I am aware and for this particular feature I think its worth the extra CLI arg.

If its desirable not to be added and the auto enable feature is accepted, I'm OK dropping it because in the end my worflow is guaranteed.

Just tell me what's desired.

@nicoddemus nicoddemus referenced this pull request Feb 5, 2018

Closed

Fixes to live logs #3175

3 of 4 tasks complete
'log_cli', default=False, type='bool',
help='enable log display during test run (also known as "live logging").')
add_option_ini(
'--log-stream',

This comment has been minimized.

@thisch

thisch Feb 5, 2018

Contributor

Why did you invent a new name? Why not --log-cli?

This comment has been minimized.

@s0undt3ch

s0undt3ch Feb 5, 2018

Contributor

Seems weird, --log-cli, and --log-stream just seemed more explicative. I have no issues making it --log-cli though...

auto_enable_log_stream = self._config.getoption('--log-cli-level')
if auto_enable_log_stream is not None:
# Explicitely enable log_cli is --log-cli-level was passed on the CLI
# It won't be automatically enabled is set on the ini file.

This comment has been minimized.

@thisch

thisch Feb 5, 2018

Contributor

@nicoddemus Are you aware of similar code (where command line options set sth automatically but not ini options) within other parts of pytest?

This comment has been minimized.

@s0undt3ch

s0undt3ch Feb 5, 2018

Contributor

The reason why it's only enabled automatically for the CLI option and not the INI option is because of the concerns raised on my initial PR.
Also, this PR automatically set's verbosity to 1 if log_cliis true...

This comment has been minimized.

@nicoddemus

nicoddemus Feb 7, 2018

Member

I'm more concerned that automatically enabling live-logs if log-cli-level is set conflicts with a valid use case (IMHO): user sets log_cli_level=DEBUG on pytest.ini so that they can just pass -o log_cli=true on the command-line and see DEBUG log messages live.

But to be honest I never used live logging, so I'm not sure even if this is an important/valid use case, so I will defer to @thisch the final verdict on this. 👍

@s0undt3ch

This comment has been minimized.

Contributor

s0undt3ch commented Feb 7, 2018

@nicoddemus we only automatically enable live logs if use passes --log-cli-level=something. Setting log_cli_level on the INI does not auto enable live logs.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Feb 7, 2018

@nicoddemus we only automatically enable live logs if use passes --log-cli-level=something. Setting log_cli_level on the INI does not auto enable live logs.

OH right thanks for pointing that out, my bad.

Then I think it makes perfectly sense for --log-cli-level=something to imply the log_cli option.

In this case can we drop the --log-cli command-line option and keep only log_cli ini option?

@s0undt3ch

This comment has been minimized.

Contributor

s0undt3ch commented Feb 7, 2018

Sure, I'll update in a couple of hours.

if auto_enable_log_stream is not None:
# Explicitely enable log_cli is --log-cli-level was passed on the CLI
# It won't be automatically enabled is set on the ini file.
config._inicache['log_cli'] = config._parser._inidict['log_cli'] = True

This comment has been minimized.

@nicoddemus

nicoddemus Feb 7, 2018

Member

I don't think we should be changing the config._* attributes, I prefer we isolate this predicate into a method and use that instead, for example:

def _stream_logs(self):
    return self._config.getoption('--log-cli-level') is not None or self._config.getini('log_cli')
@@ -0,0 +1,2 @@
Expose `log_cli` as a CLI parser option.

This comment has been minimized.

@nicoddemus

nicoddemus Feb 7, 2018

Member

As commented, let's drop this, and we can make this description more concise:

Passing `--log-cli-level` in the command-line now automatically activates live logging. 
result.stdout.fnmatch_lines([
'*test_log_cli_auto_enable*100%*',
'=* 1 passed in *=',
])

This comment has been minimized.

@nicoddemus

nicoddemus Feb 7, 2018

Member

Please also ensure that no logging message is shown:

 assert 'INFO' not in result.stdout.str()
 assert 'WARNING' not in result.stdout.str()
@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Feb 7, 2018

Sure, I'll update in a couple of hours.

Great, thanks, we appreciate it! 👍

@s0undt3ch

This comment has been minimized.

Contributor

s0undt3ch commented Feb 8, 2018

Should be ready for another review.

s0undt3ch and others added some commits Feb 5, 2018

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Feb 8, 2018

Thanks @s0undt3ch!

I took the liberty of renaming _stream_logs_enabled to _log_cli_enabled myself because I think it is clearer this way, we already have "log cli" and "live log" as aliases to each other. 😁

@s0undt3ch

This comment has been minimized.

Contributor

s0undt3ch commented Feb 8, 2018

Sure, no problem!

@s0undt3ch

This comment has been minimized.

Contributor

s0undt3ch commented on _pytest/logging.py in ad7d63d Feb 8, 2018

Actually, this is evaluated on every test, why do you prefer not to cache the result of this call?

This comment has been minimized.

Member

nicoddemus replied Feb 8, 2018

Because self._config.getoption and self._config.getini are basically dict lookups, so they are very fast; I don't think it warrants trying to optimize this. 😁

This comment has been minimized.

Contributor

s0undt3ch replied Feb 8, 2018

👌

@thisch

thisch approved these changes Feb 8, 2018

or because --log-cli-level was given in the command-line.
"""
return self._config.getoption('--log-cli-level') is not None or \
self._config.getini('log_cli')

This comment has been minimized.

@thisch

thisch Feb 8, 2018

Contributor

Thx for introducing a method for this! This improves the readability IMO.

@nicoddemus nicoddemus merged commit bba258a into pytest-dev:features Feb 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@s0undt3ch s0undt3ch deleted the s0undt3ch:feature/logs-stream branch Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment