master: Updated Diff to Catch TypeError

[Goldcap]

Added a TypeError exception, really from here:

#150

Lhanks for the code! Super handy :)

[carljm]

Thanks for the PR! We generally maintain 100% line and branch test coverage; would you be willing to add a test to the PR that exercises this addition?

[Goldcap]

Yes, but it'll be a bit, I'm up to my ears in work. Sorry for any delay, but will do ASAP and pass another PR.

Presuming it's in TimeFramedModelTests? Just glancing at the code, I can also create another class or use another if that's better.

[carljm]

The fix is to FieldTracker, so the test would go with the rest of the FieldTracker tests.

No need for a new PR, you can just add a new commit to this one (by pushing it to the same branch.)

[Goldcap]

Just as an FYI, was working on the tests, and there is some recursive test method thing happening that my tiny little brain can't get around...

======================================================================
FAIL: test_post_save_previous (model_utils.tests.tests.InheritedModelTrackerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/amadsen/html/sites/django-model-utils/model_utils/tests/tests.py", line 1438, in test_post_save_previous
    self.assertPrevious(name='retro', number=4, mutable=[1,2,3], date=testdate)
  File "/home/amadsen/html/sites/django-model-utils/model_utils/tests/tests.py", line 1334, in assertPrevious
    self.assertEqual(tracker.previous(field), value)
AssertionError: None != datetime.datetime(2001, 12, 1, 1, 0)

But then later:

======================================================================
FAIL: test_post_save_previous (model_utils.tests.tests.InheritedFieldTrackerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/amadsen/html/sites/django-model-utils/model_utils/tests/tests.py", line 1438, in test_post_save_previous
    self.assertPrevious(name='retro', number=4, mutable=[1,2,3], date=None)
  File "/home/amadsen/html/sites/django-model-utils/model_utils/tests/tests.py", line 1334, in assertPrevious
    self.assertEqual(tracker.previous(field), value)
AssertionError: datetime.datetime(2001, 12, 1, 1, 0) != None

So not sure my tests are gonna get committed too soon, only because I can't understand where this method is coming from twice, with different params.

[carljm]

Some tests are run multiple times under different conditions by inheriting TestCase classes. The test_post_save_previous test is implemented in FieldTrackerTests, which has a tracked_class class attribute. Then several other testcases inherit FieldTrackerTests (meaning they inherit all its tests), and override the tracked_class attribute to run those tests against a different tracked model with some special characteristic.

It's a cheap way to "parametrize" tests, reusing the same test under slightly different conditions to improve the overall coverage of the test suite.

Does that help clarify?

[Goldcap]

Yes, that's helpful. I must have missed the InheritedFieldTrackerTests and ModelTrackerTests. Sorry for all the back and forth, you must be hating this pull request by now :) But removing those (for now) did the trick, so back in bizness.

Oh, and one additional sidenote, this may not matter, but for me the PassThroughManager test raises a PicklingError, which I'll assume is something with my build so I'm ignoring for now.

PicklingError: Can't pickle <class 'model_utils.managers._PassThroughManager'>: it's not found as model_utils.managers._PassThroughManager

[carljm]

Not at all, I appreciate you sticking with it!

Hmm, the PicklingError is odd. Yeah, ignore it for now and we'll see if it pops up anywhere else (e.g. on Travis CI).

[Goldcap]

Added native, then TZ, aware "date" field to tests. Note, I had not earlier modified the ModelInstanceTracker, so pay special attention to this:

https://github.com/Goldcap/django-model-utils/blob/05a986042268556f2bc8916215b505e576602eb0/model_utils/tracker.py

Felt bad writing it, but had to insert the exception somehow :)

[carljm]

The change to ModelInstanceTracker looks fine. That class will actually go away soon, it only exists for legacy compatibility.

So I guess we should have talked a bit more about how to implement this test. I see that you probably ran across this issue in the context of datetimes (naive vs tz-aware), but really the change isn't inherently related to datetimes, and using datetimes in the tests makes the test changes much more invasive than I'd prefer (including a new dependence on pytz, which I'd rather not introduce, and which will cause the Travis CI run to fail).

Really all that's needed to trigger this new code-path is any object that raises a TypeError on being compared to any other object. Fortunately such an object is easy to create in Python:

>>> class Incomparable(object):
...     def __cmp__(self, other):
...             raise TypeError("I can't be compared to anything!")
... 
>>> foo = Incomparable()
>>> foo != 4
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in __cmp__
TypeError: I can't be compared to anything!

So I think the test for this can be written much more simply without needing to introduce the new date fields on the tracked models, or mess about at all with datetimes or timezones. Just add the Incomparable class, and assign an instance of Incomparable to any existing field of the tracked models, and it will have the same effect.

Sorry to ask you to make this change after all the work you did on the datetime-using approach :/

[carljm]

Let's not use dict as a variable name and shadow the built-in dict constructor. Something like data would be better.

[romgar]

In our current issue situation, it will mask the TypeError, which will be even harder to detect. I think we should properly handle timezone related issues.

[romgar]

@Goldcap quite old MR, are you still interested to work on it?

[ppandy]

@romgar Sorry, I dropped the ball on this one. I think we should probably close this out at this point.

[romgar]

No problem, I completely understand. I will probably dive into it at some point:-)

[Natim]

I see an interesting feature here. @romgar do you feel like I should try to rebase it and fix the tests?

[romgar]

You can definitely have a look @Natim if you are interested. I don't have much time these days to work on this anymore.