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

Integrate pytest-faulthandler into the core #5440

Closed
nicoddemus opened this issue Jun 12, 2019 · 10 comments

Comments

Projects
None yet
3 participants
@nicoddemus
Copy link
Member

commented Jun 12, 2019

I was wondering if we should integrate pytest-faulthandler into the core? Given that faulthandler is a builtin since Python 3.3 and we no longer support Python < 3.5 seems like this would be very helpful for users to diagnose core dumps.

It is a small plugin (less than 70 lines of code), and introduces two new options, --no-faulthandler and --faulthandler-timeout, with faulthandler being enabled by default for maximum usefulness. I've been using it in production for many years without problems.

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

If nobody opposes this, I will try to get this into 5.0. 👍

@nicoddemus nicoddemus added this to the 5.0 milestone Jun 12, 2019

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Any suggestions on how to make this more smooth for users which already depend on pytest-faulthandler, as installing pytest and pytest-faulthandler would cause a "duplicated command-line option" error?

@thisch

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

For the logging plugin we added the following code to config/__init__.py:

    def register(self, plugin, name=None):
        if name in ["pytest_catchlog", "pytest_capturelog"]:
            warnings.warn(
                PytestConfigWarning(
                    "{} plugin has been merged into the core, "
                    "please remove it from your requirements.".format(
                        name.replace("_", "-")
                    )
                )
            )
            return
            ...

Can't we do the same or sth similar for pytest-faulthandler?

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Any suggestions on how to make this more smooth for users which already depend on pytest-faulthandler, as installing pytest and pytest-faulthandler would cause a "duplicated command-line option" error?

not the best solution but I had the same problem in flake8 and here's what I did: https://github.com/PyCQA/flake8/blob/9b79c40cdb86b29cf65d7054b846da4a1b242430/src/flake8/plugins/manager.py#L254-L259

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Thanks for the suggestions (also I take you guys are OK with adding this to the core?).

Another approach came to mind, taken by conda when they integrated conda-env into their core.

For pytest this would be:

Release pytest-faulthandler 2.0 (current version is 1.6) as an empty package, requiring pytest>=5.

This way pytest <5 users get the actual plugin, and pytest>=5 users get an empty package.

Thoughts? The advantage I see here is that we don't have to pollute pytest's code with plugin-specific details.

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

That won't help the people still using old fault handler with new pytest however :S

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

so it's clear, I'm for adding this to core -- I think it's a useful plugin and pretty low impact. The testing portion might become difficult (I haven't looked at the testing situation yet, but my guess is it involves building some broken C extensions) -- though we probably already have some dependencies that already have a C compiler as a dependency

@nicoddemus nicoddemus changed the title Integrate pytest-handler into the core Integrate pytest-faulthandler into the core Jun 12, 2019

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

That won't help the people still using old fault handler with new pytest however :S

But that will be rare, I think, because people are either pinning both pytest and pytest-faulthandler versions, or not pinning at all. Or am I wrong in assuming this?

The testing portion might become difficult

Actually it is pretty simple, we can generate crashes easily using a utility in faulthandler itself meant for testing: https://github.com/pytest-dev/pytest-faulthandler/blob/1684020847fe9e8bcb664ba6ea5ac90f0e4f0fcf/test_pytest_faulthandler.py#L8-L19

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

it's fairly common for one to pin both, but only update the pin of one library at a time -- this is (for instance) the most common way packages are upgraded at lyft across our 100s of python services

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Ahh true. 👍

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 12, 2019

Integrate pytest-faulthandler into the core
* Add pytest-faulthandler files unchanged
* Adapt imports and tests
* Add code to skip registration of the external `pytest_faulthandler`
  to avoid conflicts

Fix pytest-dev#5440

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 12, 2019

Integrate pytest-faulthandler into the core
* Add pytest-faulthandler files unchanged
* Adapt imports and tests
* Add code to skip registration of the external `pytest_faulthandler`
  to avoid conflicts

Fix pytest-dev#5440

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 12, 2019

Integrate pytest-faulthandler into the core
* Add pytest-faulthandler files unchanged
* Adapt imports and tests
* Add code to skip registration of the external `pytest_faulthandler`
  to avoid conflicts

Fix pytest-dev#5440

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 12, 2019

Integrate pytest-faulthandler into the core
* Add pytest-faulthandler files unchanged
* Adapt imports and tests
* Add code to skip registration of the external `pytest_faulthandler`
  to avoid conflicts

Fix pytest-dev#5440

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 13, 2019

Integrate pytest-faulthandler into the core
* Add pytest-faulthandler files unchanged
* Adapt imports and tests
* Add code to skip registration of the external `pytest_faulthandler`
  to avoid conflicts

Fix pytest-dev#5440

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 13, 2019

Integrate pytest-faulthandler into the core
* Add pytest-faulthandler files unchanged
* Adapt imports and tests
* Add code to skip registration of the external `pytest_faulthandler`
  to avoid conflicts

Fix pytest-dev#5440

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 22, 2019

Integrate pytest-faulthandler into the core
* Add pytest-faulthandler files unchanged
* Adapt imports and tests
* Add code to skip registration of the external `pytest_faulthandler`
  to avoid conflicts

Fix pytest-dev#5440
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.