Illegal use of locals in test suite

[Former user]

According to python specification:

"""locals( )

Update and return a dictionary representing the current local symbol table. Warning: The contents of this dictionary should not be modified; changes may not affect the values of local variables used by the interpreter. """

line 66 of test/orm/extendedattr.py stands:

locals()42 = 99

Which is a bit strange and might not work on all python interpreters. What about changing it a bit more explicitely (like explicitely call setattr on a class, or modify a dict) to avoid such issues?

cheers, fijal

[Former user]

pje added that line

[Mike Bayer]

references to the debate:
http://mail.python.org/pipermail/python-dev/2008-June/080200.html and http://morepypy.blogspot.com/2008/06/running-pylons-on-top-of-pypy.html .

Considering the standoff, we might have to mark the test as "cpython" only. We'd need to build a test decorator for that.

[Former user]

It would not be a big deal (test will simply fail), but the problem is it happens on import level which prevents tests from running. On the other hand, I would like not to allow keeping strange stuff in dict of new-style classes in pypy (old style are fine). I would hope that relying on such detail (undocumented and untested) should not prevent pypy from running sqlalchemy.

Cheers, fijal

[jek]

locals() mutability it's not critical to the overall tests at all, so wrapped up in a try/except. c8de80f4ea912c27cab97309cba9c39500b6d122

[Former user]

According to: http://mail.python.org/pipermail/python-dev/2008-June/080309.html

keeping non-string keys in type dictionaries is illegal. I'm reopening the ticket, but on the other hand, it should not be considered very crucial (even for running sqlalchemy on top of pypy), since this use case is only in tests. (Although this tests something for a reason, exactly one for which pypy will fail at some point further). I'm up to discussion whether this should be fixed or won't.

Cheers, fijal

[Mike Bayer]

"fixing" this ticket as you say would then result in a new bug report "no test coverage for full AddOns compatibility", which is a more severe issue IMO. So I don't see what it accomplishes to reopen this ticket.

[Former user]

I know. So where shall I submit this ticket to whoever uses non-string keys in types dict?

[Mike Bayer]

I don't know much about AddOns but it seems strange that it has this requirement in the first place. Deeper research would probably lead over there.

[Former user]

Since this is not in any sense essential for sqlalchemy, please close it (I cannot)

[jek]

i think we're agreed now that the current setup is ok: the line only runs under cpython, and does nothing other than ensure that cpython sqla's instrumentation doesn't explode when probing classes produced by AddOns, an unrelated cpython module from pypi.

[Former user]

ekhem Sorry to reopen this again, but this try: except: does not solve anything. This locals() substitution works, but creating a class afterwards doesn't.

[Former user]

reopening

[Mike Bayer]

Since I don't have access to pypy could you be more specific about what the exact behavior is ? I tried artificially placing an exception throw within the try/except block (before or after the locals() hack) and the test still completes successfully.

[jek]

my tests on pypy show that modifying locals() seems to work just fine, so the except: is never triggered. it croaks at the end of the class definition, presumably while assembling the locals dict into the class dictionary.

[Mike Bayer]

was there a reason we didn't want to implement @unsupported('pypy') ?

[jek]

no, though this isn't located in a function block.

[Michael Trier]

What do we want to do about this one?

[Mike Bayer]

059512f264f1fb41f4201840dc0706bb526b80c4

[Mike Bayer]

Removing milestone: 0.7.0 (automated comment)