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

s0undt3ch · 2018-02-05T18:10:50Z

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 · 2018-02-05T18:11:59Z

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 · 2018-02-05T18:58:32Z

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

RonnyPfannschmidt · 2018-02-05T19:46:00Z

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 · 2018-02-05T21:42:55Z

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.

twmr · 2018-02-05T22:47:07Z

_pytest/logging.py

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


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

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

twmr · 2018-02-05T22:52:44Z

_pytest/logging.py

+        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.


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

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...

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 · 2018-02-07T20:17:27Z

@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 · 2018-02-07T20:22:01Z

@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 · 2018-02-07T20:22:47Z

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

nicoddemus · 2018-02-07T20:26:25Z

_pytest/logging.py

+        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


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')

nicoddemus · 2018-02-07T20:28:12Z

changelog/3190.feature

@@ -0,0 +1,2 @@
+Expose `log_cli` as a CLI parser option.


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.

nicoddemus · 2018-02-07T20:31:06Z

testing/logging/test_reporting.py

+        result.stdout.fnmatch_lines([
+            '*test_log_cli_auto_enable*100%*',
+            '=* 1 passed in *=',
+        ])


Please also ensure that no logging message is shown:

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

nicoddemus · 2018-02-07T20:32:42Z

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

Great, thanks, we appreciate it! 👍

s0undt3ch · 2018-02-08T09:41:54Z

Should be ready for another review.

nicoddemus · 2018-02-08T11:52:11Z

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 · 2018-02-08T12:12:26Z

Sure, no problem!

twmr · 2018-02-08T14:56:13Z

_pytest/logging.py

+        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')


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

s0undt3ch force-pushed the feature/logs-stream branch from e34bf2a to fb58033 Compare February 5, 2018 18:52

s0undt3ch force-pushed the feature/logs-stream branch from fb58033 to be2b10b Compare February 5, 2018 19:00

nicoddemus mentioned this pull request Feb 5, 2018

Fixes to live logs #3175

Closed

4 tasks

twmr reviewed Feb 5, 2018

View reviewed changes

s0undt3ch force-pushed the feature/logs-stream branch 2 times, most recently from 2dd23fe to 244096d Compare February 6, 2018 10:20

nicoddemus requested changes Feb 7, 2018

View reviewed changes

s0undt3ch force-pushed the feature/logs-stream branch 2 times, most recently from 0c5b2dd to 421a764 Compare February 8, 2018 09:41

Expose log_cli as a CLI parser option.

0b71255

s0undt3ch force-pushed the feature/logs-stream branch from 421a764 to 0b71255 Compare February 8, 2018 09:43

Rename _stream_logs_enabled to _log_cli_enabled and remove _stream_logs

ad7d63d

nicoddemus approved these changes Feb 8, 2018

View reviewed changes

twmr approved these changes Feb 8, 2018

View reviewed changes

Fix linting

a4cbd03

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

s0undt3ch deleted the feature/logs-stream branch February 8, 2018 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

s0undt3ch commented Feb 5, 2018 •

edited

Loading

s0undt3ch commented Feb 5, 2018

coveralls commented Feb 5, 2018 •

edited

Loading

RonnyPfannschmidt commented Feb 5, 2018

s0undt3ch commented Feb 5, 2018

twmr Feb 5, 2018

s0undt3ch Feb 5, 2018

twmr Feb 5, 2018

s0undt3ch Feb 5, 2018

nicoddemus Feb 7, 2018

s0undt3ch commented Feb 7, 2018

nicoddemus commented Feb 7, 2018

s0undt3ch commented Feb 7, 2018

nicoddemus Feb 7, 2018

nicoddemus Feb 7, 2018

nicoddemus Feb 7, 2018

nicoddemus commented Feb 7, 2018

s0undt3ch commented Feb 8, 2018

nicoddemus commented Feb 8, 2018

s0undt3ch commented Feb 8, 2018

twmr Feb 8, 2018 •

edited

Loading

Expose log_cli as a CLI parser option. #3190

Expose log_cli as a CLI parser option. #3190

Conversation

s0undt3ch commented Feb 5, 2018 • edited Loading

s0undt3ch commented Feb 5, 2018

coveralls commented Feb 5, 2018 • edited Loading

RonnyPfannschmidt commented Feb 5, 2018

s0undt3ch commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s0undt3ch commented Feb 7, 2018

nicoddemus commented Feb 7, 2018

s0undt3ch commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Feb 7, 2018

s0undt3ch commented Feb 8, 2018

nicoddemus commented Feb 8, 2018

s0undt3ch commented Feb 8, 2018

twmr Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

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

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

s0undt3ch commented Feb 5, 2018 •

edited

Loading

coveralls commented Feb 5, 2018 •

edited

Loading

twmr Feb 8, 2018 •

edited

Loading