tests.properties.PropertyKindsTest.test_pickle_events fails

[James Henstridge]

As of r269, the test suite fails in the test_pickle_event test.

    FAIL: test_pickle_events (tests.properties.PropertyKindsTest)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/james/src/lp/storm.bug-230977/tests/mocker.py", line 102, in test_method_wrapper
        result = test_method()
      File "/home/james/src/lp/storm.bug-230977/tests/properties.py", line 647, in test_pickle_events
        self.assertEquals(changes, [(self.variable1, None, ["a"], False)])
    AssertionError: [] != [(<storm.variables.PickleVariable object at 0x97f290>, None, ['a'], False)]

The test in question creates a class with two Pickle() columns. Using a bit of printf debugging, I noticed that the _start_tracking() was being called for each of the two variables, but only one "flush" event handler ended up being attached.

The following example demonstrates why this is so:

    >>> class Foo(object):
    ... def method(self): pass
    ... def __eq__(self, other): return True
    ... def __hash__(self): return 0
    ...
    >>> foo1 = Foo()
    >>> foo2 = Foo()
    >>> hash(foo1.method) == hash(foo2.method)
    True
    >>> foo1.method == foo2.method
    True
    >>>

In the test, both columns contained empty lists as their values. PickleVariable implements __hash__() and __eq__() such that if the values are equal, then they are equal and have the same hash (even if the value is unhashable ...).

So to safely use variable methods as event handlers, those variable classes should implement __hash__ and __eq__ based on object identity (which is the only sane way that you can implement hash() for *mutable* objects). I am not sure what the ramifications are for other bits of code though ...

[James Henstridge (jamesh)]

Looking further, having any variable types be hashable seems broken. Consider:

    >>> from storm.variables import IntVariable
    >>> v = IntVariable(42)
    >>> s = set([v])
    >>> v in s
    True
    >>> v.set(43)
    >>> v in s
    False

There seems to be a lot of code that depends on hashability and equality checks for variables, so I am not sure how this should be handled.

Speaking with Thomas on IRC, it seems that this test passed for him. This appears to be due to differences in set iteration order on 32 bit vs. 64 bit (so I guess I was lucky to notice this ...).

[Thomas Herve (therve)]

This is ready to review in the attached branch: the choice was to remove the hash/eq methods, and to change the way objects are stored in the alive cache.