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.

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

Choose distinct debug adapter ports in different tests. #17837

Merged
merged 3 commits into from
Dec 21, 2022
Merged

Choose distinct debug adapter ports in different tests. #17837

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b59a5a4
Choose distinct debug adapter ports in different tests.
benjyw Dec 17, 2022
9157c6e
Use execution slot var
benjyw Dec 17, 2022
f94c506
Move to util file
benjyw Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pants.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ extra_requirements.add = [
"pygments",
]
lockfile = "3rdparty/python/pytest.lock"
execution_slot_var = "TEST_EXECUTION_SLOT"

[test]
extra_env_vars = [
Expand Down
2 changes: 2 additions & 0 deletions src/python/pants/backend/python/goals/pytest_runner_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
build_runtime_package_dependencies,
get_filtered_environment,
)
from pants.core.subsystems.debug_adapter import DebugAdapterSubsystem
from pants.core.util_rules import config_files, distdir
from pants.core.util_rules.partitions import Partitions
from pants.engine.addresses import Address
Expand Down Expand Up @@ -108,6 +109,7 @@ def _configure_pytest_runner(
args = [
"--backend-packages=pants.backend.python",
f"--source-root-patterns={SOURCE_ROOT}",
f"--debug-adapter-port={DebugAdapterSubsystem.port_for_testing()}",
*(extra_args or ()),
]
rule_runner.set_options(args, env=env, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
Expand Down
2 changes: 2 additions & 0 deletions src/python/pants/backend/python/goals/run_python_source_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
from pants.backend.python.util_rules import local_dists, pex_from_targets
from pants.build_graph.address import Address
from pants.core.goals.run import RunDebugAdapterRequest, RunRequest
from pants.core.subsystems.debug_adapter import DebugAdapterSubsystem
from pants.engine.process import InteractiveProcess
from pants.engine.rules import QueryRule
from pants.engine.target import Target
Expand Down Expand Up @@ -186,6 +187,7 @@ def my_file():
"--backend-packages=pants.backend.python",
"--backend-packages=pants.backend.codegen.protobuf.python",
"--source-root-patterns=['src_root1', 'src_root2']",
f"--debug-adapter-port={DebugAdapterSubsystem.port_for_testing()}",
*(
(
"--python-default-run-goal-use-sandbox"
Expand Down
12 changes: 12 additions & 0 deletions src/python/pants/core/subsystems/debug_adapter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

from __future__ import annotations

import os

from pants.option.option_types import IntOption, StrOption
from pants.option.subsystem import Subsystem
from pants.util.strutil import softwrap
Expand All @@ -23,3 +25,13 @@ class DebugAdapterSubsystem(Subsystem):
default=5678,
help="The port to use when launching the server.",
)

@staticmethod
def port_for_testing() -> int:
Copy link
Member

@thejcannon thejcannon Dec 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not a fan of polluting normal code for testing sake.

Why do it this way and not using the var directly? Or better yet, in a high level contest you can monkeypatch the debugadapter default. Then tests don't need modifications and all new tests are also safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the car?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know where and how that monkeypatching would happen? I'm happy to turn this over to you if you have a clear idea, I just want these tests to not be broken...

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, typo.

And yeah I can, but I'm not in front of a tower until tuesday

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally you could use port 0 but it looks like they did not set debugpy up for this. I personally demand that of all my port-using tests in any project in any language, but it may require some extra effort here - possibly too brittle. FWICT you'd have to info-log scrape / poll :/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, alas, that was my first choice too, and I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thejcannon how about I merge this, and we (you?) can improve it in the future if the mood strikes. Right now this fixes a bug in our tests, so it is strictly better than the status quo ante.

Copy link
Member

Choose a reason for hiding this comment

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

In that case I'd rather we just inline the port selection code so we still aren't dirtying our "prod" code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to inline it directly, but I moved the function to a util.

"""Return a custom port we can use in Pants's own tests to avoid collisions.

Assumes that the env var TEST_EXECUTION_SLOT has been set. If not, all tests will use the
same port, and collisions may occur.
"""
execution_slot = os.environ.get("TEST_EXECUTION_SLOT", "0")
return 22000 + int(execution_slot)