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

unittest: do not use TestCase.debug with --pdb #6014

Closed
wants to merge 1 commit into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Oct 20, 2019

Reverts #1890, which needs to
be fixed/addressed in another way
(#5996).

Fixes #5991.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 20, 2019

Decided to revert to sane behavior, until this is addressed properly (#5996 is a start).

Reverts pytest-dev#1890, which needs to
be fixed/addressed in another way
(pytest-dev#5996).

Fixes pytest-dev#5991.
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.

This is a regression I'm afraid, we need to find a new way to properly handle skips...

@blueyed
Copy link
Contributor Author

blueyed commented Nov 2, 2019

Sure - but it was a regression / bug from the beginning.
Therefore I'd rather revert it already, and fix / re-try it later.

@blueyed blueyed requested a review from nicoddemus November 2, 2019 15:23
@blueyed
Copy link
Contributor Author

blueyed commented Nov 2, 2019

Should I do this against master maybe instead? ;)

Seriously, I am aware that it reverts a previous change, but that one is just wrong, and caused multiple (and more serious) regressions to start with.

I've came across this several times, and it had to be adjusted / fixed multiple times.
The original author never reacted to my pings, and might not even be using pytest anymore.
We should not protect this kind of "feature" (which is really a hack after all).
The proper / better way might just be to add/support "--unittest-debug" - using "--pdb" for that was just a misunderstanding to begin with (actually people are using that by default, and it causes all teardowns to be skipped then (IIRC)).

@blueyed
Copy link
Contributor Author

blueyed commented Nov 2, 2019

But since this conflicts anyway already, and me being tired of pointing it out (and people not caring), I'll just close it for now.
(I'm not using unittests myself btw)

@blueyed blueyed closed this Nov 2, 2019
@blueyed blueyed deleted the unittest-no-debug branch November 2, 2019 15:28
@blueyed
Copy link
Contributor Author

blueyed commented Nov 9, 2019

@nicoddemus
/cc @pytest-dev/core
To summarize: pytest uses .debug() in unittests, which is meant to be used only manually for debugging unittests.
Somebody came along and found that tests are being teared down already when using --pdb for post-portem debugging, which closed their GUI.
This was in 2016 already (#1890), and a constant source of issues, e.g. reported via pytest-django, but also here (the latest #5991).

After investigating, and giving it a try myself to fix it I've concluded that it is the best to revert this for now, especially since the issue mentions a specific workaround already, and the more obvious would be to just put a pdb.set_trace() where you need it (before the failure).

Anyway, what I really do not get is that "reverting this mis-feature" is being rejected as being a regression itself..!
Like I've mentioned above the original author never reacted to my concerns about this, and might not even be using pytest anymore - but still this bug is being protected from being reverted?!

I would like to raise awareness that we should strive for rather reverting a broken feature than carrying it along (there were several fixups already after #1890).

@nicoddemus
Copy link
Member

You got a point there. I'm on vacation at the moment so I can't look at the code now, but I will pick it up again when I get back.

@nicoddemus
Copy link
Member

I agree, see #5996 (comment).

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