Shorten too long test UNIX socket path by webknjaz · Pull Request #3832 · aio-libs/aiohttp

webknjaz · 2019-06-09T11:13:35Z

What do these changes do?

Make tmp unix sock path fit the kernel limits

Are there changes in behavior for the user?

Related issue number

Fixes #3572

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

tests/test_connector.py

webknjaz · 2019-06-09T13:20:41Z

@asvetlov should be good to go now. Plz test on your machine as well :)

asvetlov · 2019-06-09T13:56:35Z

tests/conftest.py

+
+    root_tmp_dir = Path('/tmp').resolve()
+    os_tmp_dir = Path(os.getenv('TMPDIR', '/tmp')).resolve()
+    original_base_tmp_path = Path(tmp_path_factory.getbasetemp())


Is Path() convertion required?

It's just nice for the unified API and paths comparison.

asvetlov

The whole PR can be replaced with PYTEST_DEBUG_TEMPROOT=/tmp in CI config, isn't it?

webknjaz · 2019-06-09T15:46:05Z

@asvetlov I wanted something more generic and detached from CI.

asvetlov · 2019-06-09T15:54:53Z

tests/conftest.py

+
+    root_tmp_dir = Path('/tmp').resolve()
+    os_tmp_dir = Path(os.getenv('TMPDIR', '/tmp')).resolve()
+    original_base_tmp_path = Path(str(tmp_path_factory.getbasetemp()))


What is the reason for Path -> str -> Path conversion?
If there is something please describe it in a comment

Yes, I do the comparison in a few places below. So I need a few Path() objects.
And str() in this line is only needed for Python 3.5 support which apparently cannot do this by its own.

asvetlov

Please consider the alternative:

Keep unix_sockname fixture. Drop all the rest: need_unix marker, IS_ constants etc.
In unix_sockname always create a socket path as /tmp/{uuid}.sock on systems with AF_UNIX, otherwise call pytest.skip(). The code should be about 5 lines long.

What do you think?

webknjaz · 2019-06-10T11:21:27Z

I think that the pytest's default place for the socket creation is better because it's better scoped. /tmp should be used as a fallback because it's harder to track the location when searching for stuff in the file system, during debugging, for example. So I'd like to keep a better-designed layout while having a fallback for corner cases.

webknjaz · 2019-06-10T11:23:17Z

P.S. In general, I like the marker more as it's cleaner.

asvetlov

Ok, please merge when green.

I don't care too much honestly but can live with the PR if you did all work already :)

Skip marks don't work in fixtures

patchback · 2022-09-06T16:32:49Z

Backport to 3.8: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 8e9e39b on top of patchback/backports/3.8/8e9e39b61e118f00029a1d9300323fdcd7f0ff67/pr-3832

Backporting merged PR #3832 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.8/8e9e39b61e118f00029a1d9300323fdcd7f0ff67/pr-3832 upstream/3.8

Now, cherry-pick PR Shorten too long test UNIX socket path #3832 contents into that branch:
```
$ git cherry-pick -x 8e9e39b61e118f00029a1d9300323fdcd7f0ff67
```
If it'll yell at you with something like fatal: Commit 8e9e39b61e118f00029a1d9300323fdcd7f0ff67 is a merge but no -m option was given., add -m 1 as follows intead:
```
$ git cherry-pick -m1 -x 8e9e39b61e118f00029a1d9300323fdcd7f0ff67
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Shorten too long test UNIX socket path #3832 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.8/8e9e39b61e118f00029a1d9300323fdcd7f0ff67/pr-3832

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback · 2022-09-06T16:32:49Z

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 8e9e39b on top of patchback/backports/3.9/8e9e39b61e118f00029a1d9300323fdcd7f0ff67/pr-3832

Backporting merged PR #3832 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.9/8e9e39b61e118f00029a1d9300323fdcd7f0ff67/pr-3832 upstream/3.9

Now, cherry-pick PR Shorten too long test UNIX socket path #3832 contents into that branch:
```
$ git cherry-pick -x 8e9e39b61e118f00029a1d9300323fdcd7f0ff67
```
If it'll yell at you with something like fatal: Commit 8e9e39b61e118f00029a1d9300323fdcd7f0ff67 is a merge but no -m option was given., add -m 1 as follows intead:
```
$ git cherry-pick -m1 -x 8e9e39b61e118f00029a1d9300323fdcd7f0ff67
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Shorten too long test UNIX socket path #3832 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.9/8e9e39b61e118f00029a1d9300323fdcd7f0ff67/pr-3832

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

asvetlov reviewed Jun 9, 2019

View reviewed changes

tests/test_connector.py Outdated Show resolved Hide resolved

asvetlov reviewed Jun 9, 2019

View reviewed changes

tests/test_connector.py Outdated Show resolved Hide resolved

webknjaz force-pushed the bugfix/unix-sock-path-too-long branch from 0306e04 to 52db4a1 Compare June 9, 2019 13:20

webknjaz changed the title ~~[WIP] Fix UNIX sock path too long in tests~~ Fix UNIX sock path too long in tests Jun 9, 2019

webknjaz marked this pull request as ready for review June 9, 2019 13:20

webknjaz mentioned this pull request Jun 9, 2019

Set up CI with Azure Pipelines #3827

Closed

asvetlov reviewed Jun 9, 2019

View reviewed changes

asvetlov reviewed Jun 10, 2019

View reviewed changes

asvetlov approved these changes Jun 10, 2019

View reviewed changes

webknjaz added 10 commits June 10, 2019 16:19

Make reusable *NIX check marker in connector tests

27b01f9

Add an exit-check @ unix_sockname fixture

5e6b2dd

Fix isort linter complaint

aee9bf4

Generate a shorter path for test UNIX sockets

c3dac51

Migrate unix_sockname fixture to conftest module

32b8957

Stringify the original base path

1e415f4

Simplify unix_sockname fixture

9e2c9f3

Keep needs_unix fixture only in conftest

ba612f3

Fix skipping in unix_sockname

7304b49

Skip marks don't work in fixtures

Calculate prefix bytes len

5825337

webknjaz force-pushed the bugfix/unix-sock-path-too-long branch from 491924d to 5825337 Compare June 10, 2019 14:19

webknjaz added 4 commits June 10, 2019 16:23

Drop unnecessary win check marker

a004470

Drop reason kwarg from pytest.skip() invocation

ca1aa02

Satisfy isort

de52806

Make make_tmp_dir Python-3.5-compatible

a74e524

This comment has been minimized.

Sign in to view

webknjaz added bug need backport labels Jun 11, 2019

webknjaz changed the title ~~Fix UNIX sock path too long in tests~~ Shorten too long test UNIX socket path Jun 11, 2019

webknjaz merged commit 8e9e39b into master Jun 11, 2019

asvetlov deleted the bugfix/unix-sock-path-too-long branch June 11, 2019 13:38

austinbutler mentioned this pull request Jun 13, 2021

Confusing error - OSError: AF_UNIX path too long - when default temporary directory is too long psf/black#2105

Open

webknjaz added backport-3.8 labels Sep 6, 2022

Uh oh!

Conversation

webknjaz commented Jun 9, 2019

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

Uh oh!

Uh oh!

webknjaz commented Jun 9, 2019

Uh oh!

asvetlov Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

webknjaz Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Jun 9, 2019

Uh oh!

asvetlov Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

webknjaz Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Jun 10, 2019

Uh oh!

webknjaz commented Jun 10, 2019

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

patchback bot commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backport to 3.8: 💔 cherry-picking failed — conflicts found

Backporting merged PR #3832 into master

Uh oh!

patchback bot commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backport to 3.9: 💔 cherry-picking failed — conflicts found

Backporting merged PR #3832 into master

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

patchback bot commented Sep 6, 2022 •

edited

Loading

patchback bot commented Sep 6, 2022 •

edited

Loading