8000 gh-133490: Add color support to remote PDB by godlygeek · Pull Request #133491 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-133490: Add color support to remote PDB #133491

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 6, 2025
Merged

gh-133490: Add color support to remote PDB #133491

merged 2 commits into from
May 6, 2025

Conversation

godlygeek
Copy link
Contributor
@godlygeek godlygeek commented May 6, 2025 •
edited by bedevere-app bot
Loading

@godlygeek godlygeek requested a review from gaogaotiantian as a code owner May 6, 2025 04:10
@godlygeek godlygeek changed the title Add color support to remote PDB gh-133490: Add color support to remote PDB May 6, 2025
@bedevere-app bedevere-app bot mentioned this pull request May 6, 2025
Closed
@godlygeek
Copy link
Contributor Author

CC @pablogsal @gaogaotiantian @ambv

@gaogaotiantian
Copy link
Member

This is why we need real integration tests. I think we should have a framework for users to specify some script and command then run the tests automatically. It's not trivial to mock this.

@gaogaotiantian
Copy link
Member

I think the code looks straightforward, but we have to have integration tests. To be honest I would expect most of the future tests written in integration form, instead of the current protocol form, because that's how people think of it.

I don't think integration test framework is difficult - we basically only need a subprocess to run some script (probably in an infinite loop). Then we need to define some commands, which can be directly passed into the attaching process by -c. We collect the stdout of the attaching process and we are basically done - in my opinion that's even simpler than what we currently have.

@godlygeek
Copy link
Contributor Author

The changes are really trivial, it's just calling _colorize.can_colorize() on the client and passing the result down to the server through a few layers. I agree that an integration test would be very good to add, but I don't want this to miss the boat for 3.14 because we can't decide how to write one. It would be very unfortunate if we added two new headline features to PDB in 3.14 (syntax highlighting and remote support) and they wind up being mutually exclusive!

@gaogaotiantian gaogaotiantian merged commit 982830c into python:main May 6, 2025
39 checks passed
@godlygeek godlygeek deleted the colorize_remote_pdb branch May 6, 2025 06:00
@godlygeek godlygeek mentioned this pull request May 6, 2025
Merged
@pablogsal
Copy link
Member

I think the code looks straightforward, but we have to have integration tests. To be honest I would expect most of the future tests written in integration form, instead of the current protocol form, because that's how people think of it.

I don't think integration test framework is difficult - we basically only need a subprocess to run some script (probably in an infinite loop). Then we need to define some commands, which can be directly passed into the attaching process by -c. We collect the stdout of the attaching process and we are basically done - in my opinion that's even simpler than what we currently have.

I don’t see integration tests as orthogonal to what we have currently. Every layer of testing helps in different ways. In my experience when an integration test fails is super difficult to know what the hell is going on and is much much trickier to get them right and synced across all build bots. You can see how often tests involving sub processes are racy. So I support this of course and is a great idea but I don’t think is either this or that. Is both

@godlygeek
Copy link
Contributor Author

In my experience when an integration test fails is super difficult to know what the hell is going on and is much much trickier to get them right and synced across all build bots.

Agreed. You can see how it took me 2 hours of wrestling with the build bots to get #133494 passing CI after I had things working locally. (And I'm not actually sure if it's being tested on macOS at all... The macOS build bots don't seem to be set up in a way that allows sys.remote_exec to work, even when it's a parent process attaching to its child.)

I definitely agree that the test suite is obviously incomplete if we don't have any tests exercising the -p argument in PDB's main and the sys.remote_exec call that it makes, but those tests are slower to execute, more fragile, harder to debug, and more likely to fail for platform-specific or environment-specific reasons than the unit tests that exercise each half at the protocol level.

Now that #133494 has landed there are some integration tests that you can base future ones off of, but I'd strongly encourage only using integration tests for things that can only be adequately tested using both a real client and a real server. Doing all future tests as integration tests sounds like a recipe for frustration.

Also, some scenarios are virtually impossible to test with an integration test - like, exercising what happens if the remote process dies half way through writing a message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaogaotiantian gaogaotiantian gaogaotiantian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@godlygeek @gaogaotiantian @pablogsal
0