In both test_regression_143 tests, mocks for validator.validate were created and bound to validate variables, but no assertions were made using them. The validate variables were like the validate_items variables, but for validator.validate instead of validator.validate_items, and unused, with no observable effects.
It is possible to add assertions for validate.call_count, alongside the existing validate_items.call_count assertions. I did this first, asserting the same number of call counts as in the corresponding validate_items assertions. If this is what is intended, then that could be kept, and the second commit in this PR dropped or reverted.
However, considering the effect of that change together with the code introduced in #149 (which completed #143) suggests that, unlike in the case of validator.validate_items, the current fact that validator.validate is called this number of times, and no more, does not appear to be a something these tests intend to claim and verify. I say that because:
The specific goal of these tests is to check that the change in #149 is and remains effective, and the key goal there is to avoid unnecessarily revalidating arrays, so validate_items is the specifically relevant function to instrument.
There are multiple places where a strictness check can prevent validate_items from being called. One is in the typed_elems property getter, which conditionally calls validate, which calls validate_items, and that's what happens in this test. But validate calls validate_items conditionally as well. A refactoring of the code could potentially cause more validate calls to happen in the non-strict case while still not increasing the call count for validate_items.
So I added that second commit to remove the validate mocks (and their assertions, which I'd added in the first commit). validate_items is of course still mocked and its call counts asserted, as before. The tests' behavior is effectively the same as before, so this PR (if not modified further) is effectively a cleanup, even though neither of the commits in it is by itself a cleanup.
I've kept both commits (rather than squashing them into one) so the reasoning is clear in the code, and also because I think that may make it easier to modify this PR if that turns out to be helpful.
In both
test_regression_143tests, mocks forvalidator.validatewere created and bound tovalidatevariables, but no assertions were made using them. Thevalidatevariables were like thevalidate_itemsvariables, but forvalidator.validateinstead ofvalidator.validate_items, and unused, with no observable effects.It is possible to add assertions for
validate.call_count, alongside the existingvalidate_items.call_countassertions. I did this first, asserting the same number of call counts as in the correspondingvalidate_itemsassertions. If this is what is intended, then that could be kept, and the second commit in this PR dropped or reverted.However, considering the effect of that change together with the code introduced in #149 (which completed #143) suggests that, unlike in the case of
validator.validate_items, the current fact thatvalidator.validateis called this number of times, and no more, does not appear to be a something these tests intend to claim and verify. I say that because:validate_itemsis the specifically relevant function to instrument.validate_itemsfrom being called. One is in thetyped_elemsproperty getter, which conditionally calls validate, which callsvalidate_items, and that's what happens in this test. Butvalidatecallsvalidate_itemsconditionally as well. A refactoring of the code could potentially cause morevalidatecalls to happen in the non-strict case while still not increasing the call count forvalidate_items.So I added that second commit to remove the
validatemocks (and their assertions, which I'd added in the first commit).validate_itemsis of course still mocked and its call counts asserted, as before. The tests' behavior is effectively the same as before, so this PR (if not modified further) is effectively a cleanup, even though neither of the commits in it is by itself a cleanup.I've kept both commits (rather than squashing them into one) so the reasoning is clear in the code, and also because I think that may make it easier to modify this PR if that turns out to be helpful.