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

Jump to bottom

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

merged 3 commits into from
Dec 21, 2022

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Dec 17, 2022

Prevents a port collision error if the two test files happen to run concurrently.

Prior to this change I was sometimes getting this error in one or both tests:

Can't listen for client connections: [Errno 48] Address already in use

@benjyw
Choose distinct debug adapter ports in different tests.
b59a5a4 
Prevents a port collision error if the two test files happen
to run concurrently.
@benjyw benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Dec 17, 2022
@benjyw benjyw requested a review from thejcannon December 17, 2022 23:08
@thejcannon
Copy link
Member

I would expect no two processes to run at the same time with --debug or --debug-adapter.

What's the behavior today?

@benjyw
Copy link
Contributor Author

benjyw commented Dec 17, 2022

This is our own tests running concurrently. This doesn't affect our users.

@thejcannon
Copy link
Member

Ah, I should review fewer PRs on mobile 😅

thejcannon
thejcannon reviewed Dec 17, 2022
View reviewed changes
src/python/pants/backend/python/goals/pytest_runner_integration_test.py Outdated
@@ -108,6 +108,9 @@ def _configure_pytest_runner(
args = [
"--backend-packages=pants.backend.python",
f"--source-root-patterns={SOURCE_ROOT}",
# NB: Each test file that tests the debug adapter should pick a unique port
# so that different test files can run concurrently without port collisions.
"--debug-adapter-port=22335",
Copy link
Member

Choose a reason for hiding this comment

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

Should we use the fancy pants env car for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The what now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, the execution slot? Definitely! Good idea

thejcannon
thejcannon reviewed Dec 18, 2022
View reviewed changes
src/python/pants/core/subsystems/debug_adapter.py Outdated
@@ -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.

@benjyw benjyw requested a review from thejcannon December 18, 2022 20:52
@benjyw benjyw merged commit bcf4f55 into pantsbuild:main Dec 21, 2022
@benjyw benjyw deleted the fix_debug_adapter_tests branch December 21, 2022 05:39
illicitonion added a commit to illicitonion/pants that referenced this pull request Dec 31, 2022
@illicitonion
Prep for 2.16.0.dev4
450f47f 
Internal PRs:

* Prepare 2.14.1rc0 ([pantsbuild#17904](pantsbuild#17904))

* Always stop on `StopIteration` in `rule_runner.run_rule_with_mocks` ([pantsbuild#17901](pantsbuild#17901))

* upgrade to Rust v1.66.0 ([pantsbuild#17893](pantsbuild#17893))

* Remove Eric from Rust dependabot config ([pantsbuild#17891](pantsbuild#17891))

* Refactor generate_github_workflows.py to prep for aarch64 support ([pantsbuild#17880](pantsbuild#17880))

* [internal] Subsystems, Options, and a Process for the `cc` backend ([pantsbuild#17844](pantsbuild#17844))

* Prepare 2.15.0rc1. ([pantsbuild#17883](pantsbuild#17883))

* Use GITHUB_PATH to modify paths between CI steps. ([pantsbuild#17882](pantsbuild#17882))

* Disable coverage in CI ([pantsbuild#17876](pantsbuild#17876))

* Split out different entry point fields, and revert `PexBinaryFieldSet` ([pantsbuild#17870](pantsbuild#17870))

* Choose distinct debug adapter ports in different tests. ([pantsbuild#17837](pantsbuild#17837))

* fix a bunch of missing JVM `rules()` dependencies ([pantsbuild#17861](pantsbuild#17861))

* DRYs up JVM subsystem registration code ([pantsbuild#17859](pantsbuild#17859))

* go: pass coverage setup config into `prepare_go_test_binary` rule ([pantsbuild#17835](pantsbuild#17835))

* go: split production of test binary from actually running it ([pantsbuild#17825](pantsbuild#17825))

* Remove stray `--remote-auth-plugin` reference. ([pantsbuild#17832](pantsbuild#17832))
@illicitonion illicitonion mentioned this pull request Dec 31, 2022
Merged
illicitonion added a commit to illicitonion/pants that referenced this pull request Dec 31, 2022
@illicitonion
Prep for 2.16.0.dev4
5bb9875 
Internal PRs:

* Prepare 2.14.1rc0 ([pantsbuild#17904](pantsbuild#17904))

* Always stop on `StopIteration` in `rule_runner.run_rule_with_mocks` ([pantsbuild#17901](pantsbuild#17901))

* upgrade to Rust v1.66.0 ([pantsbuild#17893](pantsbuild#17893))

* Remove Eric from Rust dependabot config ([pantsbuild#17891](pantsbuild#17891))

* Refactor generate_github_workflows.py to prep for aarch64 support ([pantsbuild#17880](pantsbuild#17880))

* [internal] Subsystems, Options, and a Process for the `cc` backend ([pantsbuild#17844](pantsbuild#17844))

* Prepare 2.15.0rc1. ([pantsbuild#17883](pantsbuild#17883))

* Use GITHUB_PATH to modify paths between CI steps. ([pantsbuild#17882](pantsbuild#17882))

* Disable coverage in CI ([pantsbuild#17876](pantsbuild#17876))

* Split out different entry point fields, and revert `PexBinaryFieldSet` ([pantsbuild#17870](pantsbuild#17870))

* Choose distinct debug adapter ports in different tests. ([pantsbuild#17837](pantsbuild#17837))

* fix a bunch of missing JVM `rules()` dependencies ([pantsbuild#17861](pantsbuild#17861))

* DRYs up JVM subsystem registration code ([pantsbuild#17859](pantsbuild#17859))

* go: pass coverage setup config into `prepare_go_test_binary` rule ([pantsbuild#17835](pantsbuild#17835))

* go: split production of test binary from actually running it ([pantsbuild#17825](pantsbuild#17825))

* Remove stray `--remote-auth-plugin` reference. ([pantsbuild#17832](pantsbuild#17832))
illicitonion added a commit to illicitonion/pants that referenced this pull request Dec 31, 2022
@illicitonion
Prep for 2.16.0.dev4
6d7ea46 
Internal PRs:

* Prepare 2.14.1rc0 ([pantsbuild#17904](pantsbuild#17904))

* Always stop on `StopIteration` in `rule_runner.run_rule_with_mocks` ([pantsbuild#17901](pantsbuild#17901))

* upgrade to Rust v1.66.0 ([pantsbuild#17893](pantsbuild#17893))

* Remove Eric from Rust dependabot config ([pantsbuild#17891](pantsbuild#17891))

* Refactor generate_github_workflows.py to prep for aarch64 support ([pantsbuild#17880](pantsbuild#17880))

* [internal] Subsystems, Options, and a Process for the `cc` backend ([pantsbuild#17844](pantsbuild#17844))

* Prepare 2.15.0rc1. ([pantsbuild#17883](pantsbuild#17883))

* Use GITHUB_PATH to modify paths between CI steps. ([pantsbuild#17882](pantsbuild#17882))

* Disable coverage in CI ([pantsbuild#17876](pantsbuild#17876))

* Split out different entry point fields, and revert `PexBinaryFieldSet` ([pantsbuild#17870](pantsbuild#17870))

* Choose distinct debug adapter ports in different tests. ([pantsbuild#17837](pantsbuild#17837))

* fix a bunch of missing JVM `rules()` dependencies ([pantsbuild#17861](pantsbuild#17861))

* DRYs up JVM subsystem registration code ([pantsbuild#17859](pantsbuild#17859))

* go: pass coverage setup config into `prepare_go_test_binary` rule ([pantsbuild#17835](pantsbuild#17835))

* go: split production of test binary from actually running it ([pantsbuild#17825](pantsbuild#17825))

* Remove stray `--remote-auth-plugin` reference. ([pantsbuild#17832](pantsbuild#17832))

Also:

Revert "upgrade to Rust v1.66.0 (pantsbuild#17893)"

This reverts commit babf5db.
illicitonion added a commit to illicitonion/pants that referenced this pull request Dec 31, 2022
@illicitonion
Prep for 2.16.0.dev4
e386c0d 
Internal PRs:

* Prepare 2.14.1rc0 ([pantsbuild#17904](pantsbuild#17904))

* Always stop on `StopIteration` in `rule_runner.run_rule_with_mocks` ([pantsbuild#17901](pantsbuild#17901))

* upgrade to Rust v1.66.0 ([pantsbuild#17893](pantsbuild#17893))

* Remove Eric from Rust dependabot config ([pantsbuild#17891](pantsbuild#17891))

* Refactor generate_github_workflows.py to prep for aarch64 support ([pantsbuild#17880](pantsbuild#17880))

* [internal] Subsystems, Options, and a Process for the `cc` backend ([pantsbuild#17844](pantsbuild#17844))

* Prepare 2.15.0rc1. ([pantsbuild#17883](pantsbuild#17883))

* Use GITHUB_PATH to modify paths between CI steps. ([pantsbuild#17882](pantsbuild#17882))

* Disable coverage in CI ([pantsbuild#17876](pantsbuild#17876))

* Split out different entry point fields, and revert `PexBinaryFieldSet` ([pantsbuild#17870](pantsbuild#17870))

* Choose distinct debug adapter ports in different tests. ([pantsbuild#17837](pantsbuild#17837))

* fix a bunch of missing JVM `rules()` dependencies ([pantsbuild#17861](pantsbuild#17861))

* DRYs up JVM subsystem registration code ([pantsbuild#17859](pantsbuild#17859))

* go: pass coverage setup config into `prepare_go_test_binary` rule ([pantsbuild#17835](pantsbuild#17835))

* go: split production of test binary from actually running it ([pantsbuild#17825](pantsbuild#17825))

* Remove stray `--remote-auth-plugin` reference. ([pantsbuild#17832](pantsbuild#17832))

* Log process stdout/stderr on deploy_to_s3.py failures. ([pantsbuild#17909](pantsbuild#17909))
illicitonion added a commit that referenced this pull request Jan 1, 2023
@illicitonion
Prep for 2.16.0.dev4 (#17907)
df1c5b2 
Internal PRs:

* Prepare 2.14.1rc0 ([#17904](#17904))

* Always stop on `StopIteration` in `rule_runner.run_rule_with_mocks` ([#17901](#17901))

* upgrade to Rust v1.66.0 ([#17893](#17893))

* Remove Eric from Rust dependabot config ([#17891](#17891))

* Refactor generate_github_workflows.py to prep for aarch64 support ([#17880](#17880))

* [internal] Subsystems, Options, and a Process for the `cc` backend ([#17844](#17844))

* Prepare 2.15.0rc1. ([#17883](#17883))

* Use GITHUB_PATH to modify paths between CI steps. ([#17882](#17882))

* Disable coverage in CI ([#17876](#17876))

* Split out different entry point fields, and revert `PexBinaryFieldSet` ([#17870](#17870))

* Choose distinct debug adapter ports in different tests. ([#17837](#17837))

* fix a bunch of missing JVM `rules()` dependencies ([#17861](#17861))

* DRYs up JVM subsystem registration code ([#17859](#17859))

* go: pass coverage setup config into `prepare_go_test_binary` rule ([#17835](#17835))

* go: split production of test binary from actually running it ([#17825](#17825))

* Remove stray `--remote-auth-plugin` reference. ([#17832](#17832))

* Log process stdout/stderr on deploy_to_s3.py failures. ([#17909](#17909))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsirois jsirois jsirois left review comments

@thejcannon thejcannon thejcannon approved these changes

Assignees
No one assigned
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benjyw @thejcannon @jsirois