Opened 13 months ago

Last modified 6 weeks ago

#27392 assigned Cleanup/optimization

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

Reported by: Tim Graham Owned by: za
Component: Documentation Version: master
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 (23)

comment:1 Changed 13 months ago by za

Owner: changed from nobody to za
Status: newassigned

comment:2 Changed 13 months ago by Mahendra Yadav

Owner: changed from za to Mahendra Yadav

comment:3 Changed 13 months ago by Tim Graham

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

A PR is started by za.

comment:4 Changed 13 months ago by za

Owner: changed from Mahendra Yadav to za

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

comment:5 Changed 13 months ago by Tim Graham

Has patch: set

comment:6 Changed 13 months ago by Reinout van Rees

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 Changed 12 months ago by Tim Graham <timograham@…>

In 321e94fa:

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

comment:8 Changed 12 months ago by Tim Graham

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 Changed 11 months ago by Barun Parruck

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 Changed 11 months ago by Tim Graham

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

comment:11 Changed 11 months ago by alisye

Owner: changed from za to alisye

comment:12 Changed 11 months ago by alisye

Owner: alisye deleted
Status: assignednew

comment:14 Changed 10 months ago by Eric Linden

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 Changed 10 months ago by Tim Graham

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 Changed 10 months ago by Eric Linden

Cc: coralvanda@… removed

comment:17 Changed 5 months ago by Roman Dmytriv

Owner: set to Roman Dmytriv
Status: newassigned

comment:18 Changed 5 months ago by Diogo Monteiro

Cc: diogo.monteiro12@… added

comment:19 Changed 4 months ago by Theofanis Despoudis

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 Changed 4 months ago by Tim Graham

This is continued in PR #8648.

comment:21 in reply to:  20 Changed 4 months ago by Theofanis Despoudis

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 Changed 4 months ago by Theofanis Despoudis

Owner: changed from Roman Dmytriv to Theofanis Despoudis

comment:23 Changed 4 months ago by Theofanis Despoudis

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 Changed 6 weeks ago by za

Owner: changed from Theofanis Despoudis to za

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

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