python
rename asyncio tests so they have readable names by adding a suffix to them
can we give the changed tests meaningful names and docstrings?
Originally posted by @Tinche in https://github.com/python/cpython/pull/95761#pullrequestreview-1064468343
There's a number of tests with names like test_context_1 test_context_2 or test_taskgroup_21a it's very difficult to work out how to fix them when they break
Eventually we could do a separate cleanup PR that adds docstrings (actually for tests I prefer comments, since the unittest package does something funny with docstrings) and maybe add something to the name (e.g. test_taskgroup_08_cancellation_in_body) but I am loath to clean up just the changed tests -- it will look weird (PEP 8 has a rule about respecting the local style over almost everything else: "Consistency within one module or function is the most important").
Expanding the quote in https://github.com/python/cpython/issues/95772#issuecomment-1207777589, which quotes Guido's https://github.com/python/cpython/pull/95761#issuecomment-1207557189, since the first, and essential, part of Guido's post is missing:
I know I let you do that in the past, but when we started comparing the code to the original in EdgeDb I realized that it would be better to keep the names and style consistent with the original.
This was a reaction to the following proposal:
In the spirit of the old boy scouts rule, 'Always leave the campground cleaner than you found it', can we give the changed tests meaningful names and docstrings?
Suggesting to close this and instead follow the advice of adding comments.
@erlend-aasland apologies I wasn't aiming to misquote Guido here, just to track "the separate cleanup PR"
@erlend-aasland apologies I wasn't aiming to misquote Guido here, just to track "the separate cleanup PR"
If you want to do the add-comments-cleanup, you should close this issue and create a new one which aims to do that specific cleanup; the title, the OP, and subsequent posts in this issue are all about renaming test method names.
I think someone mentioned leaving the original names as they are and just adding a suffix explaining the test. How bout that?
I think there may be some miscommunication around what a rename is. To me adding a suffix eg changing test_taskgroup_08 into test_taskgroup_08_cancellation_in_body is a rename
Yes, and renaming was discouraged. Adding explanatory docstrings or comments was encouraged, unless I misread the discussion.
The original suggestions made on the other PR was to remove the old name and make a brand new name
An issue was later discovered with this "renaming completely approach" that the tests no longer line up between the edgedb TaskGroup and the asyncio TaskGroup and actually lead to making determining the purpose of the test harder rather than easier (which was the counter the original goal of the renaming)
This ticket is tracking the project to rename the test items in a way that's compatible with all three goals in mind
- Make it easier discuss the test and to understand the intent when the test is seen in the test output (add a suffix)
- Add more context with how a particular situation could be reached and explain why TaskGroup behaves in some way (add a comment)
- Make it possible to refer between edgedb and asyncio tests to determine cross project intent (keep the test numbering)
Sorry I pressed "close with comment" by accident halfway through drafting this
I suggest you edit the OP to include this information. Being explicit is always a good thing, especially on the tracker.
This will cause more churn than benefit IMO, just creating a PR with suffixes isn't enough somebody needs to review the entire test to check if your suffix is even correct. This isn't the best use of our time so I'll close this.
