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

added code to honor --log-cli-level (issue #402) #883

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

kurt-cb
Copy link

@kurt-cb kurt-cb commented Mar 3, 2023

this fixes issue #402

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

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

  • Make sure to include reasonable tests for your change if necessary

  • We use towncrier for changelog management, so please add a news file into the changelog folder following these guidelines:

    • 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 it

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

@kurt-cb
Copy link
Author

kurt-cb commented Mar 3, 2023

This change implements a logger relay handler that pumps the log message back to the master node, which then uses the master logger to display the log events.

To use it, simply set the --log-cli-level option as in normal pytest, and it will push the log level requested.

I believe that is what was asked for in #402

oerdnj pushed a commit to isc-projects/bind9 that referenced this pull request Mar 16, 2023
This provides incremental output when test is running _without xdist_,
just like the old runner did.

With xdist the live output is not available, I believe because of
pytest-dev/pytest-xdist#402
pytest-dev/pytest-xdist#883 might help with
that, but I'm not going to hold my breath until it is available on
distros we use.
oerdnj pushed a commit to isc-projects/bind9 that referenced this pull request Mar 28, 2023
This provides incremental output when test is running _without xdist_,
just like the old runner did.

With xdist the live output is not available, I believe because of
pytest-dev/pytest-xdist#402
pytest-dev/pytest-xdist#883 might help with
that, but I'm not going to hold my breath until it is available on
distros we use.
oerdnj pushed a commit to isc-projects/bind9 that referenced this pull request Mar 30, 2023
This provides incremental output when test is running _without xdist_,
just like the old runner did.

With xdist the live output is not available, I believe because of
pytest-dev/pytest-xdist#402
pytest-dev/pytest-xdist#883 might help with
that, but I'm not going to hold my breath until it is available on
distros we use.
manu0x0 pushed a commit to isc-projects/bind9 that referenced this pull request May 22, 2023
This provides incremental output when test is running _without xdist_,
just like the old runner did.

With xdist the live output is not available, I believe because of
pytest-dev/pytest-xdist#402
pytest-dev/pytest-xdist#883 might help with
that, but I'm not going to hold my breath until it is available on
distros we use.
manu0x0 pushed a commit to isc-projects/bind9 that referenced this pull request May 23, 2023
This provides incremental output when test is running _without xdist_,
just like the old runner did.

With xdist the live output is not available, I believe because of
pytest-dev/pytest-xdist#402
pytest-dev/pytest-xdist#883 might help with
that, but I'm not going to hold my breath until it is available on
distros we use.

(cherry picked from commit d0619c7)
@nicoddemus
Copy link
Member

Hey @kurt-cb,

Thanks a lot for the contribution, creating a new event to send log events specifically is a good idea. 👍

Some extra things that would be important to have to get this merged:

  • Tests that exercise the feature, preferably under different log settings (to at least avoid regressions).
  • A Changelog entry.

Also left a few comments in the code, please take a look. 👍

src/xdist/dsession.py Outdated Show resolved Hide resolved
src/xdist/remote.py Show resolved Hide resolved
record.args = None
record.exc_info = None
record.exc_text = None
x = pickle.dumps(record)
Copy link
Member

Choose a reason for hiding this comment

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

Probably does not matter much, but we might force protocol version 4 here to ensure compatibility between different python interpreters.

Copy link

Choose a reason for hiding this comment

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

x = pickle.dumps(record, protocol=4)
or
x = pickle.dumps(record, protocol=pickle.DEFAULT_PROTOCOL)
?

Copy link
Member

Choose a reason for hiding this comment

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

are log messages pickle safe?
im "pretty" sure there some hazard when dealing with custom objects

Initialise an instance, using the passed queue.
"""
logging.Handler.__init__(self)
self.queue = queue
Copy link
Member

Choose a reason for hiding this comment

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

A bit confusing calling this queue when it is actually a WorkerInteractor instance, perhaps use worker_interactor? Also worth updating the docs.

src/xdist/remote.py Outdated Show resolved Hide resolved
@WurmD
Copy link

WurmD commented Sep 25, 2023

Hi @kurt-cb , shall we pair program to appease @nicoddemus and get this merged? (I'd like to do it for you but don't know

            if not True:  # self.respect_handler_level:
class RemoteMessageHandler(logging.Handler):
    """
    This handler sends events to a queue. Typically, it would be used together
    with a multiprocessing Queue to centralise logging to file in one process
    (in a multi-process application), so as to avoid file write contention
    between processes.
    This code is new in Python 3.2, but this class can be copy pasted into

how to resolve the comments on these two bits)

My direct contact if you wish to GMeet: wurmdario at gmail dot com

@kurt-cb
Copy link
Author

kurt-cb commented Sep 28, 2023

I gave up on this PR months ago. It was raised back in March. Anyway, the first issue, just need to get rid of the commented out code # self.respect_handler_le

The RemoteMessageHandler is based off of the Python QueueHandler that is part of python, here: https://docs.python.org/3/library/logging.handlers.html#queuehandler
Source: https://github.com/python/cpython/blob/8f324b7ecd2df3036fab098c4c8ac185ac07b277/Lib/logging/handlers.py#L1412

@kurt-cb
Copy link
Author

kurt-cb commented Sep 28, 2023

@WurmD
I addressed the items you were unsure about.

@kurt-cb
Copy link
Author

kurt-cb commented Sep 28, 2023

@WurmD as for the other issues, I don't have time right now to make those changes and test. If you would like to do it, go ahead. You can pull my branch and create a PR to cli_log. Once you push the first PR, I'll approve you as a contributor.

@WurmD
Copy link

WurmD commented Sep 29, 2023

Here @kurt-cb , would you give me permissions to push to your branch?

Screenshot 2023-09-29 at 12 11 07

@WurmD
Copy link

WurmD commented Oct 2, 2023

@WurmD as for the other issues, I don't have time right now to make those changes and test. If you would like to do it, go ahead. You can pull my branch and create a PR to cli_log. Once you push the first PR, I'll approve you as a contributor.

Give me permissions to push to your branch please? @kurt-cb

@nicoddemus
Copy link
Member

@WurmD you can just pull his changes into your own fork, and open a new PR: his original commits will still be correctly attributed.

@kurt-cb
Copy link
Author

kurt-cb commented Oct 13, 2023

@WurmD, you should be able to make a PR on my repo. Once I accept the PR I can give you rights to approve.

@kurt-cb
Copy link
Author

kurt-cb commented Oct 13, 2023

@WurmD you need to fork kurt-cb/pytest-xdist (make sure you get all branches when you do this), then checkout cli_log, make your changes , then create a PR against kurt-cb/pytest-xdist. Once you do this, I will have the option to make you a contributor. I don't think I can do anything until you do this.

@kurt-cb
Copy link
Author

kurt-cb commented Oct 13, 2023

@WurmD I figured out how to make you a collaborator. You should be able to create wormd/cli_log branch and push your changes directly to there. then you can create a PR and I will review your changes and push them back here.

@WurmD
Copy link

WurmD commented Nov 21, 2023

K, successfully PRed kurt-cb#3

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.

4 participants