Merge lp:~divmod-dev/divmod.org/829869-explicit-type-dropping into lp:divmod.org

[Tristan Seligmann]

[Jonathan Jacobs (jjacobs)]

review: Approve

[Jean-Paul Calderone (exarkun)]

If you're deleting whole test methods, that's not "relying" - that's "explicitly requiring, intentionally".

I'm not too happy about these deletions.

What happens if you call gc.collect() at the right point instead?

review: Needs Information

[Tristan Seligmann (mithrandi)]

Apparently I never responded on this merge proposal (although maybe I did on IRC or somewhere else). For the record, at the time I wrote this branch, inserting even multiple gc.collect() calls didn't help.

[Tristan Seligmann (mithrandi)]

So, to clarify: the tests I am deleting in this branch verify behaviour that a) I was unable to achieve on PyPy, b) I do not feel is particularly useful for actual application code, and c) appears useful for testing purposes, but is not actually used by any other tests. The primary motivation for deleting them is a), because I'd like to make Axiom work on PyPy, but I think b) and c) are also legitimate supporting motivations.

I suppose this does qualify as a backwards-incompatible change that should go through a deprecation phase, but I can't think of a way to actually detect that somebody is relying on this behaviour and issue a DeprecationWarning.

[Tristan Seligmann (mithrandi)]

I wrote a more detailed explanation of this branch on IRC, which really belongs here, so here you go:

<idnar> lvh: so basically, the situation is
<idnar> lvh: that branch deletes two tests that test some functionality
<idnar> lvh: the only code that makes use of that functionality in any way is those two tests; there isn't even anything else in the test suite that uses it, nevermind any application code that I'm aware of
<idnar> lvh: furthermore, making use of that functionality in some application in a useful fashion is extremely difficult, to the point where I would conjecture nobody has done so
<idnar> lvh: so that's why the branch just deletes the tests
<exarkun> the reason I am cautious, of course, is "any application code that I'm aware of"
<idnar> the functionality is: being able to define an item type with a certain type name, break all references to that item type, then define a different
<idnar> item with the same typeName
<idnar> (well, or the same item)
<exarkun> obscure functionality, to be sure
<idnar> perhaps I'm exaggerating the difficulty of using this functionality usefully
<idnar> well, I guess I should make a correction there
<idnar> "break all references" is not the thing you need to do; "ensure the Item subclass is deallocated" is the thing you need to do
<idnar> the unit test tries to accomplish this by breaking all of the references it has to the subclass, which works on CPython but not on PyPy
<lvh> idnar: Okay, so it's not less obscure than I imagined it. I can't imagine any application code either.
<lvh> (Again, that doesn't cover exarkun's valid comment about what we can imagine being irrelevant)
<idnar> I guess one alternative would be to skip the tests, but only on pypy
<exarkun> but we could always re-introduce it as a feature with an explicit API, later. I don't think anyone not in this channel has much right to complain about super obscure gc interaction behaviors in axiom changing.
<exarkun> (something like store.unregisterType(...), store.registerType(...))

[Laurens Van Houtven (lvh)]

I'm okay with merging this. Yes, it's backwards incompatible, but only in a way that nobody could actually reasonably use it previously. After discussing this even more I'm increasingly convinced that this test was testing something for the heck of testing, as opposed to an actual feature we want to support :)

review: Approve

[Tristan Seligmann (mithrandi)]

> After discussing
> this even more I'm increasingly convinced that this test was testing something
> for the heck of testing, as opposed to an actual feature we want to support :)

I'm too lazy to actually go digging in history to support this, but I believe that the test suite itself relied on this feature at one point (aside from the tests verifying the functionality directly, that is), but the tests have changed since then.

=== modified file 'Axiom/axiom/test/test_xatop.py'
--- Axiom/axiom/test/test_xatop.py	2010-04-03 12:38:34 +0000
+++ Axiom/axiom/test/test_xatop.py	2011-08-20 02:01:24 +0000
@@ -203,40 +203,6 @@
                    schema[2], schema[3], schema[4])] + schema[1:]})
 
 
-    def test_inMemorySchemaCacheReset(self):
-        """
-        The global in-memory table schema cache should not change the behavior
-        of consistency checking with respect to the redefinition of in-memory
-        schemas.
-
-        This test is verifying the behavior which is granted by the use of a
-        WeakKeyDictionary for _inMemorySchemaCache.  If that cache kept strong
-        references to item types or used a (typeName, schemaVersion) key,
-        either the second C{SoonToChange} class definition in this method would
-        fail or the schema defined by the first C{SoonToChange} class would be
-        used, even after it should have been replaced by the second definition.
-        """
-        class SoonToChange(item.Item):
-            attribute = attributes.integer()
-
-        dbpath = self.mktemp()
-        s = store.Store(dbpath)
-        SoonToChange(store=s)
-        s.close()
-
-        # This causes a Store._checkTypeSchemaConsistency to cache
-        # SoonToChange.
-        s = store.Store(dbpath)
-        s.close()
-
-        del SoonToChange, s
-
-        class SoonToChange(item.Item):
-            attribute = attributes.boolean()
-
-        self.assertRaises(RuntimeError, store.Store, dbpath)
-
-
     def test_checkOutdatedTypeSchema(self):
         """
         L{Store._checkTypeSchemaConsistency} raises L{RuntimeError} if the type
@@ -270,11 +236,10 @@
         SoonToChange(store=s)
         s.close()
 
-        # Get rid of both the type and the store so that we can define a new
-        # incompatible version.  It might be nice if closed stores didn't keep
-        # references to types, but whatever.  This kind of behavior isn't
-        # really supported, only the unit tests need to do it for now.
-        del SoonToChange, s
+        # Get rid of the cached information about this type
+        from axiom.item import _typeNameToMostRecentClass
+        del _typeNameToMostRecentClass[SoonToChange.typeName]
+        del SoonToChange
 
         class SoonToChange(item.Item):
             attribute = attributes.boolean()
@@ -353,48 +318,6 @@
                  secondary._indexNameOf(TestItem, ['bar', 'baz'])]))
 
 
-    def test_inMemoryIndexCacheReset(self):
-        """
-        The global in-memory index schema cache should not change the behavior
-        of index creation with respect to the redefinition of in-memory
-        schemas.
-
-        This test is verifying the behavior which is granted by the use of a
-        WeakKeyDictionary for _requiredTableIndexes.  If that cache kept strong
-        references to item types or used a (typeName, schemaVersion) key,
-        either the second C{SoonToChange} class definition in this method would
-        fail or the indexes on the schema defined by the first C{SoonToChange}
-        class would be used, even after it should have been replaced by the
-        second definition.
-        """
-        class SoonToChange(item.Item):
-            attribute = attributes.integer()
-
-        dbpath = self.mktemp()
-        s = store.Store(dbpath)
-
-        before = s._loadExistingIndexes()
-        SoonToChange(store=s)
-        after = s._loadExistingIndexes()
-
-        # Sanity check - this version of SoonToChange has no indexes.
-        self.assertEqual(before, after)
-
-        s.close()
-        del SoonToChange, s
-
-        class SoonToChange(item.Item):
-            attribute = attributes.boolean(indexed=True)
-
-        s = store.Store()
-        before = s._loadExistingIndexes()
-        SoonToChange(store=s)
-        after = s._loadExistingIndexes()
-        self.assertEqual(
-            after - before,
-            set([s._indexNameOf(SoonToChange, ['attribute'])]))
-
-
     def test_loadPythonModuleHint(self):
         """
         If the Python definition of a type found in a Store has not yet been