Opened 7 years ago

Closed 6 years ago

#27392 closed Cleanup/optimization (wontfix)

Remove "Tests that", "Ensures that", etc. from test docstings

Reported by: Tim Graham Owned by: Harshit Agrawal
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: reinout@…, diogo.monteiro12@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Our policy is for test docstrings to state the expected behavior that each test demonstrates. Preambles such as "Tests that" or "Ensures that" aren't necessary.

Good: "Horizontal and vertical filter widgets keep selected options on page reload."
Bad: "Tests that horizontal and vertical...".

For example:

$ grep -rI "Tests that" * | wc -l
158
$ grep -rI "Test that" * | wc -l
222
$ grep -rI "Ensure that" * | wc -l
173
$ grep -rI "Ensures that" * | wc -l
14

If you spot any other similar patterns, please correct them. The policy should also be added to docs/internals/contributing/writing-code/coding-style.txt.

Change History (25)

comment:1 by za, 7 years ago

Owner: changed from nobody to za
Status: newassigned

comment:2 by Mahendra Yadav, 7 years ago

Owner: changed from za to Mahendra Yadav

comment:3 by Tim Graham, 7 years ago

Another phrasing to remove is "Checks that" / "Check that".

A PR is started by za.

comment:4 by za, 7 years ago

Owner: changed from Mahendra Yadav to za

@Mahendra Yadav: currently I'm working on PR 7439.

comment:5 by Tim Graham, 7 years ago

Has patch: set

comment:6 by Reinout van Rees, 7 years ago

Cc: reinout@… added
Patch needs improvement: set

Marked the ticket as "patch needs improvement", see https://github.com/django/django/pull/7439#issuecomment-258603274

Mostly "just" re-running regexes with similar occurrences.

comment:7 by Tim Graham <timograham@…>, 7 years ago

In 321e94fa:

Refs #27392 -- Removed "Tests that", "Ensures that", etc. from test docstrings.

comment:8 by Tim Graham, 7 years ago

Has patch: unset
Patch needs improvement: unset

While reviewing the patch, I came across additional phrases that could be removed. There are probably more variants as well:

Tests if
Ensured that
Tests whether
Ensure
Make sure
Confirms that
Makes sure
Check to
Check we
Ensure the
Test

comment:9 by Barun Parruck, 7 years ago

Hi! I was thinking of working on this. Two questions --

  1. Do you want something like "Tests adding a FK constraint for an existing column" to go to "Adds foreign key constraint to an existing column", or "Foreign key constraint can be added to an existing column".
  1. Secondly, could I create a simple pull request with only ~10 or so fixes to get the hang of the development workflow? (To make sure I'm doing the right thing!)

comment:10 by Tim Graham, 7 years ago

For the "Tests adding" case, I'd just chop "Test". It's okay to make a work-in-progress PR.

comment:11 by alisye, 7 years ago

Owner: changed from za to alisye

comment:12 by alisye, 7 years ago

Owner: alisye removed
Status: assignednew

comment:14 by Eric Linden, 7 years ago

Cc: coralvanda@… added

There are plenty of 'regression test for #xxx' and similar things. Should those be left as is, modified to keep the reference to the fact that it's a regression test, or just get rid of that type of statement?

comment:15 by Tim Graham, 7 years ago

We use a ticket reference for obscure issues that can't be described in a docstring. Some places could be cleaned up but I wouldn't make it part of this ticket. It would be better to wait a bit before working on this, as described in an open PR.

comment:16 by Eric Linden, 7 years ago

Cc: coralvanda@… removed

comment:17 by Roman Dmytriv, 7 years ago

Owner: set to Roman Dmytriv
Status: newassigned

comment:18 by Diogo Monteiro, 7 years ago

Cc: diogo.monteiro12@… added

comment:19 by Theofanis Despoudis, 7 years ago

Is this Ticket worked on?. The PR listed here https://github.com/django/django/pull/7777 is closed. Is it ok to get correspondence for this because I would like to contribute?

comment:20 by Tim Graham, 7 years ago

This is continued in PR #8648.

in reply to:  20 comment:21 by Theofanis Despoudis, 7 years ago

Replying to Tim Graham:

This is continued in PR #8648.

Thank you, Tim.

Is it being worked on as it has been more than a month since the last updates and more conflicts may appear?

comment:22 by Theofanis Despoudis, 7 years ago

Owner: changed from Roman Dmytriv to Theofanis Despoudis

comment:23 by Theofanis Despoudis, 7 years ago

While working on the issue I also notices there are more edge cases:

  • Tests the
  • Test If
  • Test the
  • Ensures

plus more variations of Prefixes

I/m using a slightly more informative grep search

grep -rInw "Test" *

That prints whole words and line numbers

I will work on it this weekend

comment:24 by za, 6 years ago

Owner: changed from Theofanis Despoudis to za

I'll try to work on this within this week.

comment:25 by Harshit Agrawal, 6 years ago

Owner: changed from za to Harshit Agrawal

comment:26 by Tim Graham, 6 years ago

Resolution: wontfix
Status: assignedclosed

This ticket has too many poor attempts and is causing more distraction than it's worth.

Note: See TracTickets for help on using tickets.
Back to Top