New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using appropriate checks in tests #60714
Comments
The proposed patch upgrades tests to use specialized checks added in 3.1 and 3.2 (assertIsNone(x) instead assertTrue(x is None), assertLess(a, b) instead assertTrue(a < b), etc). This modern checks provide a more useful error message in case of a fail. Replaced only those checks that are not related to the tested operators. For example, assertTrue(a < b) preserved if the operator "<" is tested. |
Thanks for the work, but we don't generally make bulk changes like this. It generates churn in the codebase, and has the risk of inadvertently changing the meaning of the tests, to little actual benefit. Instead we modernize tests when we touch them for other reasons and are in a position to confirm that the changes do not change the meaning of the tests. (I realize that for most of your changes the meaning is trivially preserved...but when you make a lot of changes you are almost certain to make some mistakes...thus the resistance to doing bulk updates.) |
I understand this. I checked the patch few times, with long (more than a month) intervals between inspections. If someone wants to modernize some tests, he can turn to this patch for reference. |
If it isn't documented it should be, and you could open an issue for it. |
Patch updated to sync with tip. It is 5% less than first version because some changes already applied in other commits. |
I think there are two counterargument to leaving things alone.
That said, it *is* possible to ignore 2. and not worry about 1. until a test fails. And I agree that the original patch is too much to review. |
A third counterargument is:
Given that Serhiy has so diligently prepared and updated the patch, I'm inclined to say the codebase would be better off accepting the patch. David, can you imagine a process by which a patch like this could be acceptable? |
Here is synchronized to tip the patch which doesn't include patches for already separated children issues. It touches 129 files and modifies over 600 lines. |
Jason: yes, if it is broken down into small enough pieces for the module maintainer to be comfortable with the review :). I approved Serhiy's separate patch for test_email. I still wasn't sure about backporting, but I really don't like the 'unless' construct in test_email, so I decided to say OK. |
After chipping off yet some small separate issues, there are only 276 changed lines left in 80 files. |
Can we follow up on this please as it's referenced from bpo-9554 as well. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: