Opened 4 years ago

Last modified 4 years ago

#6620 enhancement new

Document conventions for writting test assertions.

Reported by: Tom Prince Owned by: Tom Prince
Priority: normal Milestone:
Component: core Keywords: policy
Cc: Branch: branches/test-case-assertion-conventions-6620
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

In particular:

  1. the order of expected vs. actual in assertEqual etc.
  2. How to test the message of an exception.

Change History (12)

comment:1 Changed 4 years ago by Tom Prince

(In [39132]) Use str(error) for testing exception messages.

Refs: #6393, #6620

comment:2 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/test-case-assertion-conventions-6620

(In [39133]) Branching to test-case-assertion-conventions-6620.

comment:3 Changed 4 years ago by Tom Prince

Keywords: review added
Owner: Tom Prince deleted

comment:4 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Tom Prince

Why is the order of arguments to assertEqual important enough to expand the already-longer-than-anyone-wants-to-read testing standard?

I think actual is unclear. the value produced by the unit being tested (or by the SUT to be obscure and jargony).

Also you got the preferred order backwards. :/

comment:5 Changed 4 years ago by Julian Berman

FWIW I prefer actual, result, because I find assertEqual(somestuffthatyouhavenocontextfor, thing) less readable than assertEqual(foo.bar(), thing) which reads right to left contextually, but I wouldn't really think that should be enforced.

comment:6 Changed 4 years ago by Tom Prince

Why is the order of arguments to assertEqual important enough to expand the already-longer-than-anyone-wants-to-read testing standard?

If there is a convention, then we should document it, so people can point to it, rather than it being folk-lore.

Also you got the preferred order backwards.

I don't have a strong preference either way. Part of the reason I put it there is that I knew there was a convention, but wasn't entirely sure which way it was.

Talking a random sampling of existing practise in twisted, it seems that both occur, but actual, expected has fairly significantly higher incidence.

For example:

  • 7 self.assertEqual(DeprecationWarning, warnings[0]['category']) vs. 25 self.assertEqual(warnings[0]['category'], DeprecationWarning)

Also, the idiom d.addCallback(self.assertEqual, expected) has that order of arguments. (Although whether that is a *good* idiom is another question.

comment:7 Changed 4 years ago by Jean-Paul Calderone

If there is a convention, then we should document it, so people can point to it, rather than it being folk-lore.

I agree, and thanks for running these things down. However, I don't think there's a convention for this particular thing and I'm not sure there's a lot of *value* in there being a convention. Neither form seems particularly more readable to me, though I'm sure everyone has their personal preferences (as I mentioned, mine is the opposite of what you documented).

I think this is going to end up being much more of a burden than the benefits will justify.

Perhaps if there were a tool that automatically rewrote source code to use one form or the other ...

comment:8 Changed 4 years ago by Tom Prince

(In [39161]) Remove discussion of order of arguments to assertEqual.

This convention appears controversial, so remove it for the moment. It can perhaps be added in a later ticket.

Refs: #6620

comment:9 Changed 4 years ago by Tom Prince

Keywords: review added
Owner: changed from Tom Prince to Jean-Paul Calderone

comment:10 Changed 4 years ago by Thijs Triemstra

It's assigned to exarkun but +1 from me.

comment:11 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Tom Prince

I noticed a couple of typos:

  • writting
  • typicaly

I wonder how hard it would be to set up a documentation spell checking builder?

Also, what's the point in testing static exception messages?

The only reason I can think of is when I want to ensure that the message contains certain key information eg

ResolverError('Unexpected response from server. messageID: 2345, server: 192.0.2.100')

...but then it's probably best to store the important info as attributes on the exception which can be tested separately.

Having read the recent logging discussion, maybe certain Twisted exceptions should follow the same conventions as log.msg eg

e = ResolverError(format='Unexpected response from server. messageID: %(messageID), server: %(server)', messageID=2345, server='192.0.2.100')

e.messageID
e.server

My 2c.

comment:12 Changed 4 years ago by Tom Prince

I wonder how hard it would be to set up a documentation spell checking builder?

I think the trick would be false positives, from identifiers and the like.

Note: See TracTickets for help on using tickets.