Skip to content

bpo-30190: improved error msg for assertAlmostEqual(delta=...)#1331

Merged
giampaolo merged 6 commits intomasterfrom
30190-unittest-improved-err-msg
May 1, 2017
Merged

bpo-30190: improved error msg for assertAlmostEqual(delta=...)#1331
giampaolo merged 6 commits intomasterfrom
30190-unittest-improved-err-msg

Conversation

@giampaolo
Copy link
Contributor

…e difference between the 2 numbers in case of failure
@mention-bot
Copy link

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

safe_repr(first),
safe_repr(second),
safe_repr(delta),
diff)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing safe_repr() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, thanks


if delta is not None:
if abs(first - second) <= delta:
diff = abs(first - second)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is duplicated in both branches. It could be move above the if.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

standardMsg = '%s != %s within %r places' % (safe_repr(first),
safe_repr(second),
places)
standardMsg = '%s != %s within %r places (%s difference)' % (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about assertNotAlmostEqual()?

Copy link
Contributor Author

@giampaolo giampaolo Apr 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm understanding the code wrong but assertNotAlmostEqual fails if the two numbers are almost equal and just "off" by a little. So in case of:

assertNotAlmostEqual(1.00000001, 1.0, places=7)

The failure will look like this:

AssertionError 1.00000001 == 1.0 within 7 places

...which is already enough. Makes sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How this differs from the case when delta is specified?

Actually I don't know what is the use case for assertNotAlmostEqual(). It is not used in Python tests (except testing assertNotAlmostEqual() itself). I suggest to not change it at all.

Copy link
Contributor Author

@giampaolo giampaolo Apr 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my understanding delta is entirely about acceptable difference (hence it makes sense to show the absolute diff) whereas places is about decimal precision.
I'm also not sure what's the use case for assertNotAlmostEqual() but whoever uses it will find this:

self.assertNotAlmostEqual(5, 10, delta=15)

...produces:

AssertionError: 5 == 10 within 15 delta (5 difference)

...more readable than the current message
So in summary, for assertNotAlmostEqual, I'd be for showing the difference if delta is specified but not if places is specified.

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Apr 28, 2017
@giampaolo
Copy link
Contributor Author

OK to merge?

@giampaolo giampaolo merged commit 5d7a8d0 into master May 1, 2017
@giampaolo
Copy link
Contributor Author

Merging as per Raymond's approval.

@zware zware deleted the 30190-unittest-improved-err-msg branch June 9, 2017 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants