Opened 14 months ago

Last modified 9 months 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
(diff, github, buildbot, log)
Author: tomprince Launchpad Bug:

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 14 months ago by tomprince

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

Refs: #6393, #6620

comment:2 Changed 14 months ago by tomprince

  • Author set to tomprince
  • Branch set to branches/test-case-assertion-conventions-6620

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

comment:3 Changed 14 months ago by tom.prince

  • Keywords review added
  • Owner tom.prince deleted

comment:4 Changed 14 months ago by exarkun

  • 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 14 months ago by Julian

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 14 months 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 14 months ago by exarkun

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 13 months ago by tomprince

(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 13 months ago by tom.prince

  • Keywords review added
  • Owner changed from tom.prince to exarkun

comment:10 Changed 13 months ago by thijs

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

comment:11 Changed 13 months ago by rwall

  • Keywords review removed
  • Owner changed from exarkun 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 9 months 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.