hamcrest / PyHamcrest

Hamcrest matchers for Python
http://hamcrest.org/
Other
767 stars 111 forks source link

assert_that should not call Matcher.describe_mismatch #181

Closed rittneje closed 2 years ago

rittneje commented 3 years ago

Currently assert_that will first call matches, and then if that returns false, it goes back and calls describe_mismatch. Consequently, assert_that is wastefully checking the same things twice in a row when something fails. (In fact, many Matchers implement describe_mismatch by simply calling matches.) https://github.com/hamcrest/PyHamcrest/blob/498e031815312485cb9fb54a88291d3eb5735efd/src/hamcrest/core/assert_that.py#L65-L73

Since the description is already an optional parameter to matches, it would make more sense for assert_that to pass one in the first place.

mismatch_description = StringDescription()
if not matcher.matches(actual, mismatch_description):
    full_description = StringDescription()
    full_description.append_text(reason).append_text("\nExpected: ").append_description_of(
        matcher
    ).append_text("\n     but: ").append_text(str(mismatch_description)).append_text("\n")
    raise AssertionError(full_description)
brunns commented 3 years ago

This is true, but I'm not sure that the small performance increase is worth the small potential backward incompatibility.

offbyone commented 2 years ago

Seconded. The performance of assert_that is not not-important; it's called at least once per asserted match, after all. But, I'd rather see us invest in matcher performance if we see concrete benchmarks showing that those details matter, and in that case we'll try to improve those instead.