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

Add --debug-adapter flag to test goal #15799

Merged
merged 13 commits into from Jun 14, 2022
Merged

Conversation

thejcannon
Copy link
Member

@thejcannon thejcannon commented Jun 9, 2022

Currently I'm only adding this for Python (using debugpy): others can follow suit.

This allows clients to launch their test (run coming later) with --debug-adapter which launches the DAP server before loading any Python code. The result is the user can then connect using VS Code (or any other compliant editor) and debug their tests!

[ci skip-rust]
[ci skip-build-wheels]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@@ -311,6 +318,21 @@ def activated(cls, union_membership: UnionMembership) -> bool:
"""
),
)
debug_adaptor = BoolOption(
"--debug-adaptor",
Copy link
Member Author

Choose a reason for hiding this comment

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

We should decide this, as we'll share it with run.

Comment on lines +329 to +330
The interactive process used will be immediately blocked waiting for a client before
continuing.
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll want to make sure this is true for other adaptors. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I think so too.

for debug_request, field_set in zip(debug_requests, targets_to_valid_field_sets.field_sets):
if debug_request.process is None:
if test_subsystem.debug_adaptor:
logger.info(f"Pants doesnt have an adaptor for {field_set.address}. Skipping test.")
Copy link
Member Author

Choose a reason for hiding this comment

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

Warn?

prepend_argv=(
"--listen",
f"{debugpy.host}:{debugpy.port}",
"--wait-for-client",
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, we could not have the enforcement all implementations want this, and instead make the user specify --debugpy-args=... but this is much nicer to the user.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

lgtm, but would prefer consistent use of the adapter word. :)

@thejcannon thejcannon changed the title Add --debug-adaptor flag to test goal Add --debug-adapter flag to test goal Jun 10, 2022
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Nice.

src/python/pants/core/goals/test.py Outdated Show resolved Hide resolved
src/python/pants/jvm/test/junit.py Outdated Show resolved Hide resolved
Comment on lines +329 to +330
The interactive process used will be immediately blocked waiting for a client before
continuing.
Copy link
Member

Choose a reason for hiding this comment

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

I think so too.

@kaos
Copy link
Member

kaos commented Jun 10, 2022

Heh, sorry @thejcannon but I just took a quick grep through the Pants code base, and here adaptor is way more common... so being consistent, would mean to go that way, perhaps 😁

@thejcannon
Copy link
Member Author

Actually, I went forward with adapter because it is consistent with the protocol name 😉

@kaos
Copy link
Member

kaos commented Jun 10, 2022

I prefer adapter as well... was just when I raised my gaze and found more adaptors/adapters, that I at least wanted to point it out.

$ ack daptor -ch src/python/pants/
137
$ ack dapter -ch src/python/pants/
5

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +25 to +26
register_interpreter_constraints = True
default_interpreter_constraints = ["CPython>=3.7,<3.11"]
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This is slightly odd, because this needs to safely mix with the interpreter for pytest and your code. Ideally this would inherit the exact same constraints as pytest (i.e. they need to be in the "same resolve"), but I can't picture how to do that via our existing subsystem infrastructure.

Copy link
Contributor

Choose a reason for hiding this comment

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

At a minimum, should this be set to Pytest.default_interpreter_constraints?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well this isn't test specific: #15829

src/python/pants/core/goals/test.py Outdated Show resolved Hide resolved
src/python/pants/core/goals/test.py Outdated Show resolved Hide resolved
src/python/pants/core/goals/test.py Outdated Show resolved Hide resolved
src/python/pants/jvm/test/junit.py Outdated Show resolved Hide resolved
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

@thejcannon thejcannon merged commit 704ece3 into pantsbuild:main Jun 14, 2022
@thejcannon thejcannon deleted the dap branch June 14, 2022 16:16
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Cool!

Comment on lines +25 to +26
register_interpreter_constraints = True
default_interpreter_constraints = ["CPython>=3.7,<3.11"]
Copy link
Contributor

Choose a reason for hiding this comment

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

At a minimum, should this be set to Pytest.default_interpreter_constraints?

thejcannon added a commit that referenced this pull request Jun 15, 2022
@ptrhck
Copy link

ptrhck commented Jun 24, 2022

@thejcannon This is a great feature which I would like to test. Can you share a VScode task and launch configuration to try this out?

@thejcannon
Copy link
Member Author

thejcannon commented Jun 24, 2022

@thejcannon This is a great feature which I would like to test. Can you share a VScode task and launch configuration to try this out?

The vanilla Python: Remote Attach should work out of the box for you with the default settings. Then ./pants test --debug-adapter ... and F5 from your IDE.

thejcannon added a commit that referenced this pull request Jun 27, 2022
run's counterpart to #15799. Support is added for pex_binary (which will be spiritually lifted to the python_source with #15849)

[ci skip-rust]
[ci skip-build-wheels]
thejcannon added a commit to thejcannon/pants that referenced this pull request Jun 28, 2022
run's counterpart to pantsbuild#15799. Support is added for pex_binary (which will be spiritually lifted to the python_source with pantsbuild#15849)

[ci skip-rust]
[ci skip-build-wheels]
thejcannon added a commit to thejcannon/pants that referenced this pull request Jun 28, 2022
run's counterpart to pantsbuild#15799. Support is added for pex_binary (which will be spiritually lifted to the python_source with pantsbuild#15849)

[ci skip-rust]
[ci skip-build-wheels]
stuhood pushed a commit that referenced this pull request Jun 29, 2022
`run`'s counterpart to #15799. Support is added for `pex_binary` (which will be spiritually lifted to the `python_source` with #15849)

[ci skip-rust]
[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants