Skip to content
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

Fixed #27392 -- Remove "Tests that", "Ensures that", etc. from test docstings #8648

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@roman-dmytriv
Copy link

roman-dmytriv commented Jun 16, 2017

No description provided.

roman-dmytriv added some commits Jun 16, 2017

@timgraham
Copy link
Member

timgraham left a comment

I know this is tedious but please review your work carefully to make sure the new docstrings read correctly so I don't have to make lots of corrections.

@@ -64,8 +64,8 @@ def _mocked_authenticated_request(self, url, user):

def test_select_related_preserved(self):
"""
Regression test for #10348: ChangeList.get_queryset() shouldn't
overwrite a custom select_related provided by ModelAdmin.get_queryset().
ChangeList.get_queryset() shouldn't

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

The line lengths should be reflowed to 79 characters so there isn't a short line followed by a long line.

element tags.
Also a regression test for #13599, verifies that hidden fields
element tags (#11791.
Verifies that hidden fields

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

"Verifies that" is a similar phrase to avoid.

To avoid reusing another user's session, ensure a new, empty session is
created if the existing session corresponds to a different authenticated
user.
A new, empty session is created if the existing session corresponds to a different authenticated

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

Wrap at 79 characters please.

test for #2698."""
"""
Deleting related objects should also not be distracted by a
restricted manager on the related object(#2698).

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

missing space before (

@@ -83,7 +83,7 @@ def test_basic(self):
self.assertEqual(results[0].child.name, "c1")
self.assertEqual(results[0].second_child.name, "c2")

# Regression for #16409 - make sure defer() and only() work with annotate()
# Make sure defer() and only() work with annotate() (#16409).

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

"Make sure" is a phrase to avoid

@@ -121,7 +121,7 @@ def file_upload_quota_broken(request):

def file_upload_getlist_count(request):
"""
Check the .getlist() function to ensure we receive the correct number of files.
Check the .getlist() function to receive the correct number of files.

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

This isn't grammatically correct.

If the directory name contains a dot and the file name doesn't, make
sure we still mangle the file name instead of the directory name.
sure we still mangle the file name instead of the directory name ((#9610).

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

double ((

@@ -1514,7 +1514,7 @@ def test_remove_index_state_forwards(self):

def test_alter_field_with_index(self):
"""
Test AlterField operation with an index to ensure indexes created via
AlterField operation with an index to indexes created via

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

This isn't grammatically correct.

@@ -153,7 +153,7 @@ def test_underlying_field(self):
self.assertEqual(related.uuid_fk.pk, related.uuid_fk_id)

def test_update_with_related_model_instance(self):
# regression for #24611
# (#24611).

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

Adding a useful comment for the comments that are only "# regression for #24611" might be out of the scope for this PR but I don't think the change of putting the ticket number followed by a period is an improvement for these cases.

@@ -111,8 +111,8 @@ def get_list_select_related(self, request):

def test_result_list_empty_changelist_value(self):
"""
Regression test for #14982: EMPTY_CHANGELIST_VALUE should be honored
for relationship fields
EMPTY_CHANGELIST_VALUE should be honored

This comment has been minimized.

Copy link
@timgraham

timgraham Jun 16, 2017

Member

should be -> is

@roman-dmytriv

This comment has been minimized.

Copy link
Author

roman-dmytriv commented Jun 16, 2017

Thanks for your comments. I will fix it asap.

@theodesp

This comment has been minimized.

Copy link
Contributor

theodesp commented Jul 17, 2017

Whats the status so far on this one @roman-dmytriv?

@roman-dmytriv

This comment has been minimized.

Copy link
Author

roman-dmytriv commented Jul 18, 2017

Unfortunately I didn't have enough time to complete the task.
I will have time on the next weekend or at 29-30.
And if you want to complete the task you can reassign it to yourself @theodesp .

@theodesp

This comment has been minimized.

Copy link
Contributor

theodesp commented Jul 18, 2017

Thank you @roman-dmytriv I really appreciate your help. I will take it as I have spare time this week. Is there anything I would like to know extra or I just review the changes and perform the code review fixes?

@roman-dmytriv

This comment has been minimized.

Copy link
Author

roman-dmytriv commented Jul 18, 2017

Thank you too @theodesp! Just review the changes and perform the code review fixes.

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Oct 20, 2017

Closing due to inactivity.

@timgraham timgraham closed this Oct 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.