Skip to content

Comments

Fix test event loop#2059

Closed
cecton wants to merge 1 commit intoaio-libs:2.2from
cecton:fix-test-event-loop
Closed

Fix test event loop#2059
cecton wants to merge 1 commit intoaio-libs:2.2from
cecton:fix-test-event-loop

Conversation

@cecton
Copy link
Contributor

@cecton cecton commented Jul 6, 2017

What do these changes do?

Set the current event loop properly during the test.

Are there changes in behavior for the user?

Some functions that use the default event loop will now work properly: asyncio.create_subprocess_exec and asyncio.create_subprocess_shell

Related issue number

#2058

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."

@cecton cecton force-pushed the fix-test-event-loop branch 3 times, most recently from 67c4795 to 66d35be Compare July 6, 2017 15:53
@codecov-io
Copy link

codecov-io commented Jul 6, 2017

Codecov Report

Merging #2059 into 2.2 will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              2.2    #2059   +/-   ##
=======================================
  Coverage   97.03%   97.03%           
=======================================
  Files          38       38           
  Lines        7683     7683           
  Branches     1341     1341           
=======================================
  Hits         7455     7455           
  Misses        104      104           
  Partials      124      124
Impacted Files Coverage Δ
aiohttp/test_utils.py 99.28% <100%> (ø) ⬆️

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 c02e119...66d35be. Read the comment docs.

@asvetlov
Copy link
Member

asvetlov commented Jul 10, 2017

No. We intentionally disabled default event loop to check for passing the loop explicitly everywhere in aiohttp.
This behavior should be kept at least while we support Python < 3.5.3.

But your problem has another solution: just call loop.get_child_watcher() after a new loop creation.

@cecton
Copy link
Contributor Author

cecton commented Jul 10, 2017

@asvetlov ah thanks! Makes totally sense.

Then would it be a good idea to add this call in the setup_test_loop? Because apparently I'm not the only one to have failed to understand.

@cecton
Copy link
Contributor Author

cecton commented Jul 10, 2017

Actually that won't work. get_child_watcher is a method of a policy, not a loop...

@asvetlov
Copy link
Member

asvetlov commented Jul 10, 2017

Ooops, I see.
But should be a way for attaching a new loop without making it default.

@cecton
Copy link
Contributor Author

cecton commented Jul 10, 2017

I found a way ^_^

@cecton cecton force-pushed the fix-test-event-loop branch 2 times, most recently from cf24a08 to e869be2 Compare July 10, 2017 12:57
@cecton
Copy link
Contributor Author

cecton commented Jul 10, 2017

Actually I'm not entirely sure that you want to enforce the developers to give the event loop explicitly. According to my memory of the doc of asyncio, this is highly "recommended". But enforcing it will only break all the applications that rely on that behavior. For once, I'm the one who suggest to be flexible on that.

@cecton
Add automatically a SafeChildWatcher to the test loop
3398a09 
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 e869be2 to 3398a09 Compare July 10, 2017 13:25
@cecton
Copy link
Contributor Author

cecton commented Jul 10, 2017 •
edited
Loading

I close this PR and re-open it for master because it will now be a feature and not a fix.

@cecton cecton closed this Jul 10, 2017
@cecton cecton mentioned this pull request Jul 10, 2017
Merged
5 tasks
@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

outdated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cecton @codecov-io @asvetlov