◐ Shell
reader mode source ↗
Skip to content

bpo-16510: Using more specific assertions in miscellaneous tests#792

Closed
serhiy-storchaka wants to merge 1 commit into
python:masterfrom
serhiy-storchaka:misc-tests-asserts
Closed

bpo-16510: Using more specific assertions in miscellaneous tests#792
serhiy-storchaka wants to merge 1 commit into
python:masterfrom
serhiy-storchaka:misc-tests-asserts

Conversation

@serhiy-storchaka

Copy link
Copy Markdown
Member

No description provided.

@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement needs backport to 2.7 tests Tests in the Lib/test dir labels Mar 23, 2017
@mention-bot

Copy link
Copy Markdown

@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @freddrake, @loewis and @florentx to be potential reviewers.

@rhettinger

Copy link
Copy Markdown
Contributor

I still contend that most of these should not be done. Unless the test is failing, there is no reason to suspect that a different error message is needed (i.e. it is unlikely that we will ever see a benefit from even a tiny fraction of this huge number of changes). When we make changes to the code, we have our tests as a safety net, but when making wholesale changes to the test suite itself, there is no safety net (i.e. if you make a mistake, we silently lose the value of the test). The changes are not being made as Guido suggested (he has a strong preference for holistic refactoring where the work is done while focusing on the needs and use cases of a particular module with full understanding of the ramifications of the changes in that module, rather than sweeping broadly changing many dozens of files, thinking only of the desire to use specific asserts rather than studying one module as time, focusing on its needs). In at least some of the cases, the new error messages add no more information than was done before. I like that simple assertTrue cases have a virtue in making it exactly clear what code is being exercised, which stands in contrast to some of the "specific asserts" do their tests indirectly. This is especially problematic for the standard library where we are testing the language itself. For example, in testing sets, assertTrue(a < b) is trying to actually exercise the "<" operator. Replacing this with assertLessThan(a, b) obfuscates the test and make me wonder whether someone got creative with unittest and was calling "b > a" or "not a >= b" or some such. As the writer of the test and as a reader, I want to know for certain that the operator was exercised (in addition "<" doesn't even mean "less than" for sets). Another issue, is that I have written many tests in a test-driven-development style. I have made a failing test and then made it pass. That makes the test sacred. Once your refactoring is done, I would have far less confidence that the test is working as intended. Likewise, use of "specific asserts" is a stylistic choice and it is unreasonable to inflict your choice on everyone else, altering the tests someone else wrote in a way that made sense to them. Personally, I find some of the specific asserts to be jarring and don't align well with what I was thinking when I wrote the tests. Lastly, if you decide to ignore me yet again, please don't backport these changes. There is no argument to be made that these are bug fixes; in fact, they just destabilize existing releases, creating a risk of introducing a buggy test.

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

I disagree with most of your arguments. Special tests for operators (like assertTrue(a < b)) are intentionally left unchanged. The fact that some old tests provide almost the same additional information in error messages doesn't negate the fact that most of old tests don't provide such information. Even tests that include additional information in error messages can get a gain from using specialized assertion method due more human-readable output (too old strings are truncated). And all tests would look more clean, this allows the reader to focus on the test itself. Backporting these changes would help to backport new tests or test fixes in the future. Sometimes new tests are not backported because testing code is different too much.

But I don't want to argue.

@serhiy-storchaka serhiy-storchaka deleted the misc-tests-asserts branch April 5, 2017 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Tests in the Lib/test dir type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants