Skip to content

Comments

Add automatically a SafeChildWatcher to the test loop#2075

Merged
asvetlov merged 1 commit intoaio-libs:masterfrom
cecton:fix-test-event-loop
Jul 25, 2017
Merged

Add automatically a SafeChildWatcher to the test loop#2075
asvetlov merged 1 commit intoaio-libs:masterfrom
cecton:fix-test-event-loop

Conversation

@cecton
Copy link
Contributor

@cecton cecton commented Jul 10, 2017

The fact that on Unix system we don't create automatically a child
watcher makes all call to asyncio.create_subprocess_* to fail.

What do these changes do?

It adds a default child watcher to the test loop.

Are there changes in behavior for the user?

Any asyncio.create_subprocess_* call in the application or during a test will work because the ChildWatcher exist (on Unix).

No change for Windows.

Related issue number

#2058 #2059

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.bug)
    • 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."

@codecov-io
Copy link

codecov-io commented Jul 10, 2017

Codecov Report

Merging #2075 into master will decrease coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2075      +/-   ##
==========================================
- Coverage   97.08%   97.07%   -0.02%     
==========================================
  Files          38       38              
  Lines        7689     7695       +6     
  Branches     1344     1345       +1     
==========================================
+ Hits         7465     7470       +5     
  Misses        102      102              
- Partials      122      123       +1
Impacted Files Coverage Δ
aiohttp/test_utils.py 98.94% <83.33%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a57797c...78bac63. Read the comment docs.

@cecton
Copy link
Contributor Author

cecton commented Jul 10, 2017

@asvetlov ready for your review


@asyncio.coroutine
def test_subprocess_co(loop):
skip_if_on_windows()
Copy link
Member

Choose a reason for hiding this comment

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

Please replace it with @pytest.skipif decorator.
I pretty sure keeping a check alongside with test improves readability.

The fact that on Unix system we don't create automatically a child
watcher makes all call to asyncio.create_subprocess_* to fail.
@cecton cecton force-pushed the fix-test-event-loop branch from 78b503e to 78bac63 Compare July 12, 2017 09:43
@cecton
Copy link
Contributor Author

cecton commented Jul 19, 2017

@asvetlov this is ready for your review

@asvetlov asvetlov merged commit 8e76a4a into aio-libs:master Jul 25, 2017
@asvetlov
Copy link
Member

Cool!

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bot:chronographer:provided There is a change note present in this PR outdated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants