Clarifying assertIs style guideline

Django Internals
1

Django’s style guide says “for testing boolean values” to prefer assertIs (matching the examples in the tutorial), but in practice the situation is more nuanced. Per this PR discussion I’m proposing a clarification intended to save some review cycles.

As I understand it, the rationale for preferring assertIs is:

  • it rigorously only accepts the boolean True, not other truthy expressions.

This may be another:

  • Discouraging assertTrue as a rule of thumb encourages more appropriate assertions, e.g assertEqual(len(qs), 1) versus assertTrue(len(qs) == 1), which may be tempting for those with pytest backgrounds. 0 != 1 is more informative output than False is not true.

But I’m suggesting the following example is unnecessarily precise because the boolean is not produced by Django:

self.assertIs(os.path.exists(app_path), True)

Others have suggested this, too. (I’ll mention we even have this case written the other way with assertTrue in our suite, but to be clear, I’m not suggesting any sort of audit of existing tests and would suggest closing a PR doing so.)

If we clarify this point in the style guide, then some contributors could save a trip there entirely, and those we do refer there would get a more nuanced rationale.

Something like:

diff --git a/docs/internals/contributing/writing-code/coding-style.txt b/docs/internals/contributing/writing-code/coding-style.txt
index 20605aef56..b0c075c436 100644
--- a/docs/internals/contributing/writing-code/coding-style.txt
+++ b/docs/internals/contributing/writing-code/coding-style.txt
@@ -110,9 +110,11 @@ Python style
   expression matching.
 
   Use :meth:`assertIs(…, True/False)<unittest.TestCase.assertIs>` for testing
-  boolean values, rather than :meth:`~unittest.TestCase.assertTrue` and
-  :meth:`~unittest.TestCase.assertFalse`, so you can check the actual boolean
-  value, not the truthiness of the expression.
+  boolean values produced by Django, rather than :meth:`~unittest.TestCase.assertTrue`
+  and :meth:`~unittest.TestCase.assertFalse`, so you can check the actual
+  boolean value, not the truthiness of the expression. (For boolean values
+  produced elsewhere, e.g. the Python standard library, either assertion is
+  acceptable.)
 
 * In test docstrings, state the expected behavior that each test demonstrates.
   Don't include preambles such as "Tests that" or "Ensures that".

Thoughts?

3 Likes
2

Many thanks for your proposal!

Sorry for nitpicking, but maybe the “under our control” aspect could be explained more clearly?
For example:

  • boolean values produced by Django code, …
  • boolean values produced by the Django implementation, …
  • boolean values produced by other code within the Django framework, …
  • boolean values produced by code that belongs to the Django framework, …
3

Thanks @jacobtylerwalls , this makes sense.

Agree here. I think I prefer the phrasing “boolean values produced by Django”. The docs normally talk about Django’s code, as “Django doing things”.

4

Thank you, @jacobtylerwalls, for starting this thread. I agree with the sentiment. Personally, I find assertIs less elegant, likely due to my longstanding use of assertTrue/assertFalse.

That said, the code review process must also be considered. Any construct/rule open to interpretation creates extra work for reviewers and may lead to debates over what qualifies as “a boolean produced by Django.” Also, an “if this then that” rule increases the cognitive load for new contributors (or even for those returning after a long absence).

In many cases, assertIs has successfully caught truthy values returned due to subtle bugs and misunderstandings of the expected type.

For these reasons, I prefer that we stick with the current rule. Although somewhat rigid, it is unambiguous, eases the review process, and has proven robust, and could even be automated via a lint check.