search

Yelp / bravado-core

Other
109 stars 98 forks source link

Fix model isinstance regression #345

Closed macisamuele closed 5 years ago

macisamuele commented 5 years ago

The changes in https://github.com/Yelp/bravado-core/commit/9603483c348b9ba00d1288ba88aeef4a62c7e155 introduced __isinstancecheck__ regression.

The following test would fail, while it should pass (as a Dog is not a Cat)

def test_(polymorphic_spec):
    dog = polymorphic_spec.definitions['Dog']._unmarshal({'name': 'name', 'type': 'Dog', 'birth_date': '2017-11-02'})
    assert isinstance(dog, polymorphic_spec.definitions['Cat']) is False

The regression is caused by the fact that due to the fact that I've adopted slots hasattr(self, '_model_spec') would always be returned True so I updated it to type(self) == ModelMeta. The interesting point is that type(self) is always going to be ModelMeta so there would be a very fast short circuit in the logic. This highlights that we do not have a great coverage of "edge" cases on that but it could have been noticed by observing the coverage drop (v5.12.1 vs v5.13.0).

In order to fix the regression we might use two alternative approaches:

  1. revert the breaking change -> this would remove the slots advantage
  2. update the __instancecheck__ logic and define more accurate tests coverage

I opted for the second solution as it would allow us to save space while keeping an accurate determination of isinstance.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.3%) to 98.887% when pulling 445e812283cc4556eccbb400630cd3af1dbfa3cb on macisamuele:maci-fix-model-isinstance-regression into d448fe3a55776bb2f6d8521957978e0e599fce24 on Yelp:master.

macisamuele commented 5 years ago

I'm surprised by the AppVeyor failure considering that the equivalent build on my AppVeyor profile was successful: https://ci.appveyor.com/project/macisamuele/bravado-core/builds/25119125/job/jbc8hlx34xtudmia vs https://ci.appveyor.com/project/sjaensch/bravado-core/build/job/2uwq50fg9vc55pg1

I'll try to do some digging to understand if there might be some flakiness

Edit: I did try to reproduce multiple times on a windows machine without any success. As our main supported platform is not windows and I was not able to partially replicate the issue, I've trigger a new build of the same changes an as far as I case see AppVeyor is happy now