Skip to content

Logging: Make pytest_runtest_logreport() hook available for logging#4812

Merged
nicoddemus merged 1 commit intopytest-dev:masterfrom
mitzkia:logging_from_runtest_logreport
Feb 22, 2019
Merged

Logging: Make pytest_runtest_logreport() hook available for logging#4812
nicoddemus merged 1 commit intopytest-dev:masterfrom
mitzkia:logging_from_runtest_logreport

Conversation

@mitzkia
Copy link

@mitzkia mitzkia commented Feb 20, 2019

Signed-off-by: Andras Mitzki andras.mitzki@balabit.com

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mitzkia!

Looking good so far, but we still need:

  • A test

  • A new changelog entry. I suggest:

    Logging messages inside ``pytest_runtest_logreport()`` are now properly captured and displayed.

@mitzkia
Copy link
Author

mitzkia commented Feb 21, 2019

@nicoddemus, Thank you for the help. Sorry, but yesterday I was in a little hurry (just wanted to run my code on CI). In this PR I also wanted to check if any other hook is related here.

@mitzkia
Copy link
Author

mitzkia commented Feb 21, 2019

As I check the following hooks can be also affected:

* pytest_runtestloop()
* pytest_runtest_makereport()
* pytest_runtest_protocol()

After I have added the missing methods to logging.py, the logging will work for the following hooks also:

* pytest_runtest_makereport()
* pytest_runtest_protocol()

Finally I would rewrite this PR title and extend the fix/tests/doc to the following hooks:

* pytest_runtest_logreport()
* pytest_runtest_makereport()
* pytest_runtest_protocol()

@mitzkia
Copy link
Author

mitzkia commented Feb 21, 2019

About the unit-tests:

  • may I extend an already existing testcase (with my new tests): test_reporting.py: test_log_in_hooks(), or should I create a new testcase?
  • in this testcase (test_log_in_hooks()) I found that pytest_runtestloop() hook should also work (but I reported, it does not work). I will check this again.

@mitzkia
Copy link
Author

mitzkia commented Feb 21, 2019

ok, sorry it did work. So pytest_runtestloop() hook is already fine.

@mitzkia
Copy link
Author

mitzkia commented Feb 21, 2019

After understanding that there are still other hooks, I just check again all of them (there are at all 52 hooks).
Logging is not work in the following hooks (except I already found):

* pytest_addhooks
* pytest_addoption
* pytest_assertrepr_compare
* pytest_cmdline_main
* pytest_cmdline_parse
* pytest_cmdline_preparse
* pytest_collect_directory
* pytest_configure
* pytest_deselected
* pytest_doctest_prepare_content
* pytest_enter_pdb
* pytest_fixture_post_finalizer
* pytest_fixture_setup
* pytest_ignore_collect
* pytest_internalerror
* pytest_itemstart
* pytest_keyboard_interrupt
* pytest_leave_pdb
* pytest_load_initial_conftests
* pytest_logwarning
* pytest_make_parametrize_id
* pytest_report_teststatus
* pytest_unconfigure
* pytest_warning_captured

@mitzkia
Copy link
Author

mitzkia commented Feb 21, 2019

May I fix all of them?

@mitzkia
Copy link
Author

mitzkia commented Feb 21, 2019

After checking some of the hooks, the fix will not easy (for all of them). There are some hooks which are not belongs to any class...

@nicoddemus
Copy link
Member

Hi @mitzkia,

May I fix all of them?

Many hook calls are nested (for example, pytest_runtest_loop calls pytest_runtest_setup and others), so if we are already capturing logs for a calling hook it is not needed to also enable log capturing for a hook called by it.

@nicoddemus
Copy link
Member

A quick glance shows that pytest_runtest_logreport indeed was missing, giving that we are log capturing pytest_runtest_logstart and pytest_runtest_logfinish.

Other hooks might be difficult/impossible because they are called very early on the plugin system, and would require a deeper look.

I suggest we just wrap pytest_runtest_logreport for now then. 👍

@mitzkia mitzkia force-pushed the logging_from_runtest_logreport branch from 53187d3 to c8deb03 Compare February 21, 2019 14:44
@mitzkia
Copy link
Author

mitzkia commented Feb 21, 2019

Hi @nicoddemus,

Thanks for the explanations. I have updated my PR. I hope the CI will passed :)

@mitzkia mitzkia force-pushed the logging_from_runtest_logreport branch from c8deb03 to de91ef8 Compare February 21, 2019 15:31
@codecov
Copy link

codecov bot commented Feb 21, 2019

Codecov Report

Merging #4812 into master will decrease coverage by 0.56%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4812      +/-   ##
==========================================
- Coverage   95.75%   95.19%   -0.57%     
==========================================
  Files         113      113              
  Lines       25653    25158     -495     
  Branches     2505     2495      -10     
==========================================
- Hits        24564    23949     -615     
- Misses        770      875     +105     
- Partials      319      334      +15
Flag Coverage Δ
#docs ?
#doctesting ?
#linting ?
#linux 93.93% <100%> (-1.65%) ⬇️
#nobyte ?
#numpy 41.11% <100%> (-51.89%) ⬇️
#pexpect 41.11% <100%> (-0.86%) ⬇️
#pluggymaster ?
#py27 92.32% <100%> (-1.35%) ⬇️
#py34 ?
#py35 ?
#py36 ?
#py37 91.9% <100%> (-1.91%) ⬇️
#py38 ?
#trial 41.11% <100%> (-51.89%) ⬇️
#windows 92.95% <100%> (-0.73%) ⬇️
#xdist 93.59% <100%> (-0.25%) ⬇️
Impacted Files Coverage Δ
testing/logging/test_reporting.py 100% <100%> (ø) ⬆️
src/_pytest/logging.py 94.96% <100%> (-0.25%) ⬇️
src/_pytest/pytester.py 80.93% <0%> (-6.25%) ⬇️
testing/test_parseopt.py 90.4% <0%> (-4.43%) ⬇️
src/_pytest/assertion/util.py 93.3% <0%> (-4.34%) ⬇️
testing/test_pdb.py 95.37% <0%> (-3.73%) ⬇️
testing/deprecated_test.py 96.77% <0%> (-3.23%) ⬇️
src/_pytest/doctest.py 93.47% <0%> (-2.93%) ⬇️
src/_pytest/compat.py 93.96% <0%> (-2.57%) ⬇️
testing/python/integration.py 89.36% <0%> (-2.37%) ⬇️
... and 76 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 da30596...b26b731. Read the comment docs.

@mitzkia mitzkia force-pushed the logging_from_runtest_logreport branch from de91ef8 to e9faa9a Compare February 21, 2019 20:15
Signed-off-by: Andras Mitzki <andras.mitzki@balabit.com>
@mitzkia mitzkia force-pushed the logging_from_runtest_logreport branch from e9faa9a to b26b731 Compare February 22, 2019 04:15
@mitzkia
Copy link
Author

mitzkia commented Feb 22, 2019

I assume the change/PR is correct (final) for now.
As I checked the CI fails on different tests / different places (not related to this PR).
Could you please validate it?

Thank you.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mitzkia! Indeed the failures are unrelated, I'm pushing a new PR to fix them shortly. 👍

@nicoddemus nicoddemus merged commit 5b35241 into pytest-dev:master Feb 22, 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.

2 participants