improver icon indicating copy to clipboard operation
improver copied to clipboard

Proposal: test module documentation title convention removal

Open cpelley opened this issue 1 year ago • 1 comments

Raised in https://github.com/metoppv/improver/pull/2000#discussion_r1625656917. This issue is to capture discussion around the removal of test module description of the file it tests. An example:

https://github.com/metoppv/improver/blob/dee4f9b1ac20b6c3adfab0d124fdc5be54b6e344/improver_tests/utilities/test_cube_checker.py#L5

Why remove?

Is the test module description of the file it tests not superfluous? Each function or class has an associated test file which is indicated by its name and directory path. I just wonder whether such conventions are necessary for the tests (the more conventions, the more time spent discussing and reviewing them). I think anything we can do to reduce the burden on the review the better (hence the benefits of tools like black, flake8 etc.).

What this proposal is not

This does not intend to imply a convention for prohibiting test module documentation. As with all modules, if there is merit to module documentation, then by all means write it.

Issues

  • https://github.com/MetOffice/improver_suite/issues/2214

cpelley avatar Jun 06 '24 07:06 cpelley

Sounds like a reasonable idea to me.

bayliffe avatar Jun 07 '24 09:06 bayliffe

I've had a think about the proposal and I think @cpelley has a good point. If it's just saying it's a test (which is obvious because it literally says it in the name and is the unit testing part of the code) then we probably don't need to then write it again. If it's something that isn't obvious it might be necessary but otherwise I'm happy 👍

Katie-Howard avatar Jul 23 '24 13:07 Katie-Howard

Thanks @Katie-Howard, ping @gavinevans I think we have enough agreement to consider this 'decided'. I have checked the developer guidelines concerning testing in https://github.com/metoppv/improver/blob/master/doc/source/Code-Style-Guide.rst#unit-testing and I don't think any update is required as a result of this decision. Thanks all.

cpelley avatar Jul 23 '24 13:07 cpelley