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

Proposed by Tristan Seligmann
Status: Merged
Merged at revision: 2713
Proposed branch: lp:~divmod-dev/divmod.org/829869-explicit-type-dropping
Merge into: lp:divmod.org
Prerequisite: lp:~divmod-dev/divmod.org/829866-explicit-close
Diff against target: 109 lines (+4/-81)
1 file modified
Axiom/axiom/test/test_xatop.py (+4/-81)
To merge this branch: bzr merge lp:~divmod-dev/divmod.org/829869-explicit-type-dropping
Reviewer Review Type Date Requested Status
Laurens Van Houtven Approve
Jean-Paul Calderone Needs Information
Jonathan Jacobs Approve
Glyph Lefkowitz Pending
Review via email: mp+72276@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Jonathan Jacobs (jjacobs) :
review: Approve
Revision history for this message
Jean-Paul Calderone (exarkun) wrote :

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
Revision history for this message
Tristan Seligmann (mithrandi) wrote :

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.

Revision history for this message
Tristan Seligmann (mithrandi) wrote :

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.

Revision history for this message
Tristan Seligmann (mithrandi) wrote :

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(...))

Revision history for this message
Laurens Van Houtven (lvh) wrote :

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
Revision history for this message
Tristan Seligmann (mithrandi) wrote :

> 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Axiom/axiom/test/test_xatop.py'
2--- Axiom/axiom/test/test_xatop.py 2010-04-03 12:38:34 +0000
3+++ Axiom/axiom/test/test_xatop.py 2011-08-20 02:01:24 +0000
4@@ -203,40 +203,6 @@
5 schema[2], schema[3], schema[4])] + schema[1:]})
6
7
8- def test_inMemorySchemaCacheReset(self):
9- """
10- The global in-memory table schema cache should not change the behavior
11- of consistency checking with respect to the redefinition of in-memory
12- schemas.
13-
14- This test is verifying the behavior which is granted by the use of a
15- WeakKeyDictionary for _inMemorySchemaCache. If that cache kept strong
16- references to item types or used a (typeName, schemaVersion) key,
17- either the second C{SoonToChange} class definition in this method would
18- fail or the schema defined by the first C{SoonToChange} class would be
19- used, even after it should have been replaced by the second definition.
20- """
21- class SoonToChange(item.Item):
22- attribute = attributes.integer()
23-
24- dbpath = self.mktemp()
25- s = store.Store(dbpath)
26- SoonToChange(store=s)
27- s.close()
28-
29- # This causes a Store._checkTypeSchemaConsistency to cache
30- # SoonToChange.
31- s = store.Store(dbpath)
32- s.close()
33-
34- del SoonToChange, s
35-
36- class SoonToChange(item.Item):
37- attribute = attributes.boolean()
38-
39- self.assertRaises(RuntimeError, store.Store, dbpath)
40-
41-
42 def test_checkOutdatedTypeSchema(self):
43 """
44 L{Store._checkTypeSchemaConsistency} raises L{RuntimeError} if the type
45@@ -270,11 +236,10 @@
46 SoonToChange(store=s)
47 s.close()
48
49- # Get rid of both the type and the store so that we can define a new
50- # incompatible version. It might be nice if closed stores didn't keep
51- # references to types, but whatever. This kind of behavior isn't
52- # really supported, only the unit tests need to do it for now.
53- del SoonToChange, s
54+ # Get rid of the cached information about this type
55+ from axiom.item import _typeNameToMostRecentClass
56+ del _typeNameToMostRecentClass[SoonToChange.typeName]
57+ del SoonToChange
58
59 class SoonToChange(item.Item):
60 attribute = attributes.boolean()
61@@ -353,48 +318,6 @@
62 secondary._indexNameOf(TestItem, ['bar', 'baz'])]))
63
64
65- def test_inMemoryIndexCacheReset(self):
66- """
67- The global in-memory index schema cache should not change the behavior
68- of index creation with respect to the redefinition of in-memory
69- schemas.
70-
71- This test is verifying the behavior which is granted by the use of a
72- WeakKeyDictionary for _requiredTableIndexes. If that cache kept strong
73- references to item types or used a (typeName, schemaVersion) key,
74- either the second C{SoonToChange} class definition in this method would
75- fail or the indexes on the schema defined by the first C{SoonToChange}
76- class would be used, even after it should have been replaced by the
77- second definition.
78- """
79- class SoonToChange(item.Item):
80- attribute = attributes.integer()
81-
82- dbpath = self.mktemp()
83- s = store.Store(dbpath)
84-
85- before = s._loadExistingIndexes()
86- SoonToChange(store=s)
87- after = s._loadExistingIndexes()
88-
89- # Sanity check - this version of SoonToChange has no indexes.
90- self.assertEqual(before, after)
91-
92- s.close()
93- del SoonToChange, s
94-
95- class SoonToChange(item.Item):
96- attribute = attributes.boolean(indexed=True)
97-
98- s = store.Store()
99- before = s._loadExistingIndexes()
100- SoonToChange(store=s)
101- after = s._loadExistingIndexes()
102- self.assertEqual(
103- after - before,
104- set([s._indexNameOf(SoonToChange, ['attribute'])]))
105-
106-
107 def test_loadPythonModuleHint(self):
108 """
109 If the Python definition of a type found in a Store has not yet been

Subscribers

People subscribed via source and target branches

to all changes: