LoggingPlugin: Support to customize log_file from hook by mitzkia · Pull Request #4752 · pytest-dev/pytest

mitzkia · 2019-02-08T22:32:46Z

First of all thanks to @Thisch who allowed to make this PR possible.

Without this patch there was only one static way to customize log_file path: --log-file starter option.
This patch allows to customize log_file value also from hooks, which helps to create log-files dynamically even for each testcases.

blueyed · 2019-02-08T22:44:56Z

blueyed added some commits 5 hours ago

Looks like this is not really based on features, but on master (or similar).

Please rebase it on features, and squash your commits into a single one.

blueyed · 2019-02-08T22:45:44Z

 * ``log_file_format``
 * ``log_file_date_format``

+You can call ``set_log_filename()`` to customize log_file path dinamically::


Suggested change

You can call ``set_log_filename()`` to customize log_file path dinamically::

You can call ``set_log_filename()`` to customize the log_file path dynamically::

Thank you for the review. The typo is fixed, and also commits are squashed and rebased.

mitzkia · 2019-02-09T11:51:10Z

Question:
For now there is a logic in set_log_filename() where we creates dir (if it is not exist) for our logfile. But we uses os.path.. form which is not compatible with Path() objects (I prefer this type).
I can fix this with checking type of fname in set_log_filename() and creating dir in proper way.
Would it be fine?

RonnyPfannschmidt · 2019-02-09T12:39:58Z

i believe for consistency with old python we need to the function to be called set_log_path, and cast it to `Path objects

in case we get a relative path, we need to choose what we use as base as well,

nicoddemus · 2019-02-09T15:14:22Z

in case we get a relative path, we need to choose what we use as base as well,

config.rootdir seems like a good candidate for that.

About the parameter, it should just be str, no need to require it to be Path objects.

RonnyPfannschmidt · 2019-02-09T15:19:38Z

@nicoddemus the parameter must support being cast to a Path, it must not be a Path

codecov · 2019-02-09T16:06:57Z

Codecov Report

Merging #4752 into features will decrease coverage by <.01%.
The diff coverage is 87.09%.

@@             Coverage Diff              @@
##           features    #4752      +/-   ##
============================================
- Coverage     95.67%   95.67%   -0.01%     
============================================
  Files           113      113              
  Lines         25041    25125      +84     
  Branches       2483     2491       +8     
============================================
+ Hits          23959    24039      +80     
- Misses          762      767       +5     
+ Partials        320      319       -1

Flag	Coverage Δ
#docs	`29.61% <47.36%> (+0.08%)`	⬆️
#doctesting	`29.61% <47.36%> (+0.08%)`	⬆️
#linting	`29.61% <47.36%> (+0.08%)`	⬆️
#linux	`95.5% <87.09%> (-0.03%)`	⬇️
#nobyte	`92.23% <87.09%> (-0.09%)`	⬇️
#numpy	`93.08% <87.09%> (-0.05%)`	⬇️
#pexpect	`42.01% <47.36%> (+0.04%)`	⬆️
#pluggymaster	`93.74% <87.09%> (?)`
#py27	`93.71% <87.09%> (ø)`	⬆️
#py34	`91.93% <87.09%> (-0.03%)`	⬇️
#py35	`91.96% <87.09%> (-0.01%)`	⬇️
#py36	`91.97% <87.09%> (-0.02%)`	⬇️
#py37	`93.84% <87.09%> (+0.01%)`	⬆️
#trial	`93.08% <87.09%> (-0.05%)`	⬇️
#windows	`93.76% <87.09%> (-0.02%)`	⬇️
#xdist	`93.69% <87.09%> (-0.09%)`	⬇️

Impacted Files	Coverage Δ
testing/logging/test_reporting.py	`100% <100%> (ø)`	⬆️
src/_pytest/logging.py	`94.91% <78.94%> (-0.51%)`	⬇️
src/_pytest/debugging.py	`77.98% <0%> (-2.81%)`	⬇️
src/_pytest/fixtures.py	`97.91% <0%> (ø)`	⬆️
src/_pytest/config/__init__.py	`93.8% <0%> (ø)`	⬆️
src/_pytest/main.py	`96.01% <0%> (ø)`	⬆️
testing/test_collection.py	`99.8% <0%> (ø)`	⬆️
testing/test_conftest.py	`99.62% <0%> (+0.01%)`	⬆️
testing/test_pdb.py	`99.07% <0%> (+0.04%)`	⬆️
src/_pytest/python.py	`92.79% <0%> (+0.12%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f672b7e...5bb3af3. Read the comment docs.

mitzkia · 2019-02-09T16:10:17Z

@RonnyPfannschmidt , @nicoddemus Thanks for the review notes.

I have added some fixes:

rename from set_log_filename() -> set_log_path() (also in docs)
I forgot that logging.FileHandler() can deal with only str instead of Path(), so I have added a check for this, and cast Path() objects to str.

Next I will add checking for abs/rel path and fix the base path if needed.

nicoddemus · 2019-02-09T16:47:30Z

        )

+    def set_log_path(self, fname):
+        if isinstance(fname, pathlib.PurePath):


I think just fname = Path(fname) is enough instead of this check. 👍

Sorry, did not understand why to cast vica versa?
At the end we will need str type for fname to work correctly within logging.FileHandler().
fname parameter can be str, or pathlib type. If it is str type everything is ok, logging.FileHandler() will work. But if it is pathlib type I would cast it to str at beginning of this function.
Please correct me if I am wrong. Thanks

Sorry, Just read: https://docs.python.org/3/library/logging.handlers.html#logging.FileHandler,
From 3.6 it supports Path() also. But I have an exception if filename in pathlib type

Maybe for compatibility reasons it is ok if the type is str

we should use pathlib and cast to str until we drop suport for python < 3.6

Thanks, fixed

nicoddemus · 2019-02-09T16:48:40Z

+        if isinstance(fname, pathlib.PurePath):
+            fname = str(fname)
+
+        if not os.path.exists(os.path.dirname(fname)):


Following my advice above, it is just a matter of changing this to fname.parent.mkdir(exist_ok=True, parents=True)

Thanks, fixed

nicoddemus

It would be nice for @Thisch to give his final say here. 👍

nicoddemus · 2019-02-09T23:05:04Z

Also thanks a ton to @mitzkia for the contribution! 🤗

twmr · 2019-02-10T20:12:30Z

Thank you very much @mitzkia for working on this!

@nicoddemus mentioned in #4707 (comment) that this feature should be marked as experimental. Not sure how this can be done. Maybe @nicoddemus can comment on what needs to be done to mark this feature as experimental.

Please do not forget to add an unittest to this PR.

nicoddemus

Please add a test case for this method; possibly making a test with the code you mention in the docs to ensure we don't break it by accident in the future.

nicoddemus · 2019-02-11T14:22:25Z

 * ``log_file_format``
 * ``log_file_date_format``

+You can call ``set_log_path()`` to customize the log_file path dynamically::


I think it is enough here to mention that this feature is experimental.

Thank you, fixed

I think we should add the example hook (that was part of 77b0dd3) once this feature is no longer experimental.

Thanks @Thisch. I am agreed with you. I will take a note for myself about this. Is there a best practice to not forgot this kind of future changes?

I would simply create a ticket for this.

nicoddemus · 2019-02-11T14:23:07Z

            log_cli_handler, formatter=log_cli_formatter, level=log_cli_level
        )

+    def set_log_path(self, fname):


Please write a docstring here given that this is now public. Also mention that this method is still experimental. 👍

Thank you, fixed.

mitzkia · 2019-02-11T20:24:45Z

I have fixed latest review notes and also added working unit test.

nicoddemus

Thanks @mitzkia! We just need to update the imports and I think we are good to go!

nicoddemus · 2019-02-11T20:48:15Z

        assert "sessionfinish" in contents
+
+def test_log_set_path(testdir):
+    from pathlib2 import Path


This needs to be from _pytest.pathlib import Path so it works for all Python versions we support.

Sorry, about that. Previously I already used that way (as you mentioned), just forgot. Soon I will fix.

nicoddemus · 2019-02-11T20:48:23Z

+    testdir.makeconftest(
+        """
+            import pytest
+            from pathlib2 import Path


This needs to be from _pytest.pathlib import Path so it works for all Python versions we support.

Thanks the review notes, I will fix this also

mitzkia · 2019-02-12T07:31:16Z

I know about TravisCI failing, soon I will fix it.

RonnyPfannschmidt · 2019-02-12T09:12:45Z

@mitzkia thanks for the followup, you are doing great, please sort things out at a pace you are most comfortable with

mitzkia · 2019-02-15T10:21:52Z

Sorry, but I can not figure out what could be the last CI error on Windows, I have already tried many possible fixes, but none of them worked. Unfortunately I have not got such Windows environment to try it.

Thanks a lot for the help.

nicoddemus · 2019-02-15T12:29:01Z

@mitzkia sure thing!

The problem is that you are inlining the full path name into the source code:

  File "C:\Users\appveyor\AppData\Local\Temp\1\pytest-of-appveyor\pytest-0\popen-gw0\test_log_set_path0\conftest.py", line 7
    report_file = os.path.join("C:\Users\appveyor\AppData\Local\Temp\1\pytest-of-appveyor\pytest-0\popen-gw0\test_log_set_path0", item._request.node.name)

And writing report_file = os.path.join("C:\User... (without escaping) produces that error. I've changed to use repr() while writing the source (see my previous commit).

nicoddemus

Thanks @mitzkia!

- This patch allows to set log_file (path) from hook Signed-off-by: Thomas Hisch Signed-off-by: Andras Mitzki <andras.mitzki@balabit.com>

mitzkia · 2019-02-15T15:07:00Z

@nicoddemus Thank you very much for the help and explanation.

mitzkia · 2019-02-15T15:07:24Z

I have updated the branch.

mitzkia mentioned this pull request Feb 8, 2019

Customizing FileHandler of LoggingPlugin from hook #4707

Closed

mitzkia force-pushed the logging_set_filename branch from 932f8db to ac8a186 Compare February 8, 2019 22:40

mitzkia force-pushed the logging_set_filename branch from ac8a186 to f038795 Compare February 8, 2019 22:46

blueyed reviewed Feb 8, 2019

View reviewed changes

mitzkia force-pushed the logging_set_filename branch 2 times, most recently from 3c8216a to fd5ce26 Compare February 8, 2019 22:50

mitzkia force-pushed the logging_set_filename branch from fd5ce26 to 5235bb8 Compare February 9, 2019 16:05

mitzkia force-pushed the logging_set_filename branch from 5235bb8 to 9170608 Compare February 9, 2019 16:26

nicoddemus reviewed Feb 9, 2019

View reviewed changes

mitzkia force-pushed the logging_set_filename branch 3 times, most recently from fa0985a to 77b0dd3 Compare February 9, 2019 20:32

nicoddemus approved these changes Feb 9, 2019

View reviewed changes

mitzkia mentioned this pull request Feb 10, 2019

light: Switch to native logger syslog-ng/syslog-ng#2546

Merged

3 tasks

nicoddemus requested changes Feb 11, 2019

View reviewed changes

mitzkia force-pushed the logging_set_filename branch from 77b0dd3 to 71e3545 Compare February 11, 2019 20:22

nicoddemus requested changes Feb 11, 2019

View reviewed changes

mitzkia force-pushed the logging_set_filename branch 2 times, most recently from 4b5ef37 to bc3bdd4 Compare February 11, 2019 21:50

mitzkia force-pushed the logging_set_filename branch 4 times, most recently from eae2d87 to 9b5ad2e Compare February 13, 2019 07:55

nicoddemus approved these changes Feb 15, 2019

View reviewed changes

LoggingPlugin: Expose setting log_file_handler

e3824d2

- This patch allows to set log_file (path) from hook Signed-off-by: Thomas Hisch Signed-off-by: Andras Mitzki <andras.mitzki@balabit.com>

mitzkia force-pushed the logging_set_filename branch from 5bb3af3 to e3824d2 Compare February 15, 2019 15:05

nicoddemus merged commit 986dd84 into pytest-dev:features Feb 16, 2019

junhaoliao mentioned this pull request Jan 5, 2026

feat(integration-tests): Clean up logging and error reporting in integration tests. y-scope/clp#1802

Merged

3 tasks

	You can call ``set_log_filename()`` to customize log_file path dinamically::
	You can call ``set_log_filename()`` to customize the log_file path dynamically::

Uh oh!

Conversation

mitzkia commented Feb 8, 2019

Uh oh!

blueyed commented Feb 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitzkia commented Feb 9, 2019

Uh oh!

RonnyPfannschmidt commented Feb 9, 2019

Uh oh!

nicoddemus commented Feb 9, 2019

Uh oh!

RonnyPfannschmidt commented Feb 9, 2019

Uh oh!

codecov Bot commented Feb 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mitzkia commented Feb 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Feb 9, 2019

Uh oh!

twmr commented Feb 10, 2019

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twmr Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitzkia commented Feb 11, 2019

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Feb 9, 2019 •

edited

Loading

mitzkia commented Feb 9, 2019 •

edited

Loading

twmr Feb 11, 2019 •

edited

Loading

nicoddemus commented Feb 15, 2019 •

edited

Loading