Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-32604: PEP 554 for use in test suite#19985
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.
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
bpo-32604: PEP 554 for use in test suite #19985
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nanjekyejoannah commented May 7, 2020 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
nanjekyejoannah commented May 8, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I intentionally left out support for |
nanjekyejoannah commented May 8, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
ohh waiting on send and recv is non trivial I guess like in https://github.com/python/cpython/pull/19829/files . |
ericsnowcurrently left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, @nanjekyejoannah! I left a number of comments for you. Hopefully it helps. I'd be glad to chat about any of this if you have questions.
FYI, I haven't looked closely at the tests yet.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented May 8, 2020
When you're done making the requested changes, leave the comment: |
vstinner commented May 15, 2020 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
nanjekyejoannah left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericsnowcurrently, I made the requested changes and I think I left some notes on making id fields read-only for SendChannel and RecvChannel. And about the high-level implementation of Exceptions. well, we can merge a minimal version and I can follow up with more PRs after feature freeze.
ericsnowcurrently left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to make those changes, @nanjekyejoannah. I've left a few more comments, but it mostly looks good. Like you said, we can add onto this after it is merged.
One other thing I recommend doing in this PR is leaving a comment at each place where the implementation is blocked on low-level implementation (e.g. channel_send_wait()).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
nanjekyejoannah commented May 16, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@ericsnowcurrently It looks like we can not directly use Does this look good to you? |
ericsnowcurrently commented May 18, 2020
@nanjekyejoannah, sorry I wasn't more clear. I meant _NOT_SET=object() classRecvChannel: ... defrecv_nowait(self, default=_NOT_SET): """ Like recv(), but return the default instead of waiting. """ifdefaultis_NOT_SET: return_interpreters.channel_recv(self._id) else: return_interpreters.channel_recv(self._id, default) |
ericsnowcurrently left a comment • edited by nanjekyejoannah
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by nanjekyejoannah
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making more changes. Sorry for adding extra work. It looks like there's only a handful of things left to do and we can merge this:
- sort out
_NOT_SETfor recv_nowait()` - add comments for the pieces that are not implemented yet in the low-level module
- remove the changes (2 added exceptions) in
Modules/_xxsubinterpretersmodule.c
Other than that, I've left a bunch of comments for formatting (PEP 8) changes (though they don't relate to correctness).
I considered recommending that we move the testing helpers to test.support, but we can worry about that later. 🙂
I also considered recommending that we limit the tests on the new interpreters module to just verifying integration with the low-level module. However, there is merit to having functional tests here. So what you have looks good.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
nanjekyejoannah commented May 18, 2020
I have made the requested changes; please review again |
bedevere-bot commented May 18, 2020
Thanks for making the requested changes! @ericsnowcurrently: please review the changes made to this pull request. |
ericsnowcurrently left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. There are a few things to adjust, particularly once we finish the low-level module. However, what you have here gives us nearly everything else. Thanks for working on this!
Feel free to merge this after Łukasz lets everyone know that beta1 is done (and the 3.9 branch created). I expect that will be tomorrow.
nanjekyejoannah commented May 19, 2020
Thanks for the review @ericsnowcurrently . I will merge this tommorrow. |
* PEP 554 for use in test suite * 📜🤖 Added by blurb_it. * Fix space * Add doc to doc tree * Move to modules doc tree * Fix suspicious doc errors * Fix test__all * Docs docs docs * Support isolated and fix wait * Fix white space * Remove undefined from __all__ * Fix recv and add exceptions * Remove unused exceptions, fix pep 8 formatting errors and fix _NOT_SET in recv_nowait() Co-authored-by: nanjekyejoannah <joannah.nanjekye@ibm.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
pablogsal commented May 26, 2020
Seems that https://buildbot.python.org/all/#/builders/394/builds/94 ...... |
pablogsal commented May 26, 2020
This reverts commit 9d17cbf.
bedevere-bot commented Jun 4, 2020
|
bedevere-bot commented Jun 4, 2020
|
…)" (GH-20611) * PEP 554 for use in test suite * 📜🤖 Added by blurb_it. * Fix space * Add doc to doc tree * Move to modules doc tree * Fix suspicious doc errors * Fix test__all * Docs docs docs * Support isolated and fix wait * Fix white space * Remove undefined from __all__ * Fix recv and add exceptions * Remove unused exceptions, fix pep 8 formatting errors and fix _NOT_SET in recv_nowait() * Update Lib/test/support/interpreters.py Co-authored-by: Pablo Galindo <Pablogsal@gmail.com> * Remove documentation (module is for internal use) Co-authored-by: nanjekyejoannah <joannah.nanjekye@ibm.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Issue #22 is about making use of subinterpreters (specifically the PEP 554 API) in the CPython test suite.
Until PEP 554 is accepted (or if it isn't) we should add a PEP 554 implementation as Lib/test/support/_interpreters.py (e.g. test.support._interpreters).
https://bugs.python.org/issue39881