classification
Title: Incomplete json tests
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.3, Python 3.2, Python 3.1, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ezio.melotti Nosy List: benjamin.peterson, bob.ippolito, doerwalter, ezio.melotti, fdrake, jowillia, pitrou, python-dev, r.david.murray, rhettinger, xuanji
Priority: high Keywords: needs review, patch

Created on 2009-04-08 15:43 by pitrou, last changed 2011-05-14 03:55 by ezio.melotti. This issue is now closed.

Files
File name Uploaded Description Edit
issue5723.diff ezio.melotti, 2011-05-12 05:45 Patch against 3.2. review
issue5723-2.diff ezio.melotti, 2011-05-13 05:39 review
Messages (20)
msg85770 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-08 15:43
Looking at the tests it seems that the pure-Python paths of json are
partly untested. In particular, py_make_scanner (as oppose to
c_make_scanner).
msg85771 - (view) Author: Bob Ippolito (bob.ippolito) * (Python committer) Date: 2009-04-08 15:51
Is this high priority? The pure-Python code paths don't even run in 
cpython. I test them manually with simplejson by just deleting the 
extension and then running the tests again. There doesn't seem to be a 
very good way to do this sort of thing
msg85772 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-08 16:01
> Is this high priority? The pure-Python code paths don't even run in 
> cpython. I test them manually with simplejson by just deleting the 
> extension and then running the tests again. There doesn't seem to be a 
> very good way to do this sort of thing

The main reason I've put it as "high priority" is that right now I'm
porting the new json to py3k, and I can't know whether the pure Python
paths are ported correctly. That probably won't refrain us from
committing it, especially if you say that they are never run with
CPython.
msg85773 - (view) Author: Walter Dörwald (doerwalter) * (Python committer) Date: 2009-04-08 16:04
test_quopri has a decorator that calls a test using both the C and
Python version of the tested function. This decorator looks like this:

def withpythonimplementation(testfunc):
    def newtest(self):
        # Test default implementation
        testfunc(self)
        # Test Python implementation
        if quopri.b2a_qp is not None or quopri.a2b_qp is not None:
            oldencode = quopri.b2a_qp
            olddecode = quopri.a2b_qp
            try:
                quopri.b2a_qp = None
                quopri.a2b_qp = None
                testfunc(self)
            finally:
                quopri.b2a_qp = oldencode
                quopri.a2b_qp = olddecode
    newtest.__name__ = testfunc.__name__
    return newtest

Adding such a decorator to every test method might solve the problem.
msg85774 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-04-08 16:07
It is a priority because we need solid test coverage in order to
successfully port 2.7 to 3.1 without breaking code or changing
semantics.  The original 3.0 port was done badly.
msg85837 - (view) Author: Bob Ippolito (bob.ippolito) * (Python committer) Date: 2009-04-10 00:50
I don't think the decorator approach would work for the doctests, it looks 
like it could be an interesting approach though. I have a feeling that 
it's going to have to be done in some kind of ugly subclass though, I'll 
dig into unittest deeper this weekend to see how that might be done.
msg85843 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-11 09:09
Hi,

> I don't think the decorator approach would work for the doctests, it looks 
> like it could be an interesting approach though. I have a feeling that 
> it's going to have to be done in some kind of ugly subclass though, I'll 
> dig into unittest deeper this weekend to see how that might be done.

Doctests will be annoying indeed. I never use doctests so I can't
suggest you anything.
As for standard unit tests, the common idiom is something like:

class JSONEncodingTests:
    def test_encode1(self):
        self.assertEquals(self.encode("foo"), "bar")
    # etc.

class CJSONEncodingTests(JSONEncodingTests, unittest.TestCase):
    encode = json.c_encode

class PyJSONEncodingTests(JSONEncodingTests, unittest.TestCase):
    encode = json.py_encode

(I'm CC'ing you since bugs.python.org looks down)

Regards

Antoine.
msg110101 - (view) Author: Fred L. Drake, Jr. (fdrake) (Python committer) Date: 2010-07-12 15:48
This lack of tests is an issue for Python 2.6 as well.

Issue 9233 might have been avoided were the pure-Python implementation tested.
msg133644 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-04-13 07:18
Some tests for py_make_scanner have been added in c3ad883b940b.

I agree that having the tested method as an attribute of the class and changing it on a different subclass is the best approach, but it's not currently done by the json tests.
Do you think the test should be refactored to use this approach?  This will also make easier to skip _json-specific tests when _json is not available and for other Python implementations.
msg133932 - (view) Author: Bob Ippolito (bob.ippolito) * (Python committer) Date: 2011-04-17 17:01
I did this some time ago in simplejson by defining a TestSuite subclass and instrumenting simplejson so that speedups can be enabled and disabled easily with a private API.

https://github.com/simplejson/simplejson/blob/master/simplejson/tests/__init__.py
msg135817 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-12 05:45
Attached patch refactors the tests to use import_fresh_module and different subclasses for Python and C tests.
It also includes a fix to import_fresh_module to make it work with packages (it can be committed separately).
msg135826 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-12 11:31
Comments:
- I don't like the fact that skip_unless_cjson() uses unittest internals. Why can't you write something like:
   skip_unless_cjson = skipUnless(...)

- instead of "self.mod", "self.json" would be nicer

- you could also export "self.loads", "self.dumps" for easier access

- you could also have two base classes exporting all this instead of repeating the attribute-setting for every test class
msg135858 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-12 20:02
> Why can't you write something like:skip_unless_cjson = skipUnless(...)

This indeed works -- using unittest internals was just a temporary workaround because the example in the unittest doc didn't seem to work.

> - instead of "self.mod", "self.json" would be nicer

I thought about using self.json, but then opted for 'mod' because is what the other modules seem to use, but I will fix it.

> - you could also export "self.loads", "self.dumps" for easier access

Usually they are not called more than a couple of times for each test, and each test class usually has 1-2 tests methods, so I'm not sure it's worth it.

- you could also have two base classes exporting all this instead of repeating the attribute-setting for every test class

I considered this too, but since the C test classes currently inherit from the Python classes, the C base class would have to be a mixin that overrides the effect of the Python base class -- unless I move all the tests in separate base classes and create two separate subclasses for each C/Python test that inherit from the base test classes and either the C or Python base classes. So the two base test classes will be in __init__:
  class CTest(TestCase):
      self.json = cjson; self.loads = cjson.loads; ...
  class PyTest(TestCase):
      self.json = pyjson; self.loads = pyjson.loads; ...

and the other test files will use either:
  class TestPySomething(PyTest):
      def test_something(self): ...
  class TestCSomething(TestPySomething, CTest):
      pass

or:
  class TestSomething(TestCase):
      def test_something(self): ...
  class TestPySomething(TestSomething, PyTest): pass
  class TestCSomething(TestSomething, CTest): pass

Another option is to have a single base class that sets self.loads/dumps in the __init__ but that will still require the module to be set in the subclasses, something like:
  class JsonTestCase(TestCase):
      def __init__(self):
          self.loads = self.json.loads
          self.dumps = self.json.dumps

and then use:
  class TestPySomething(JsonTestCase):
      json = pyjson
      def test_something(self): ...
  class TestCSomething(TestPySomething):
      json = cjson

I'm not sure any of these options is better than what we have now though.
msg135872 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-12 21:56
>   class TestSomething(TestCase):
>       def test_something(self): ...
>   class TestPySomething(TestSomething, PyTest): pass
>   class TestCSomething(TestSomething, CTest): pass

I was thinking about that. That looks clean and explicit to me.
msg135880 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-12 22:35
With this approach is necessary to exclude the base class from the tests, either by listing all the Python/C tests explicitly or doing some automatic check to find these base classes.  Listing all the tests is a bad idea because it needs to be updated manually and it's easy to forget about that and end up with tests that are never run.  Checking and skipping the base classes is not very elegant IMHO.

It also requires an extra base class, and even if it's more flexible because it makes possible to add Python-specific tests easily, that's not necessary with json because all the tests run unchanged on both pyjson and cjson.
msg135881 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-12 22:47
> With this approach is necessary to exclude the base class from the
> tests, either by listing all the Python/C tests explicitly or doing
> some automatic check to find these base classes.

It just needs a small change then:

class PyTest(TestCase):
    ...
class CTest(TestCase):
    ...

class TestSomething:
    def test_something(self): ...

class TestPySomething(TestSomething, PyTest): pass
class TestCSomething(TestSomething, CTest): pass
msg135883 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-05-12 23:23
My usual pattern (adopted from examples in the stdlib tests) is this:

TestSomethingBase:

    tests

PyTestSomething(TestSomethingBase, TestCase):
    stuff

CTestSomething(TestSomethingBase, TestCase):
    stuff

Is there a reason that won't work in your case?
msg135884 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-12 23:32
Technically they both work, they are just two different approaches that offer more or less the same compromise between features and verbosity.
Your approach requires an extra class for each test but saves you from setting the module attribute and the skip, mine is the other way around.
msg135889 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-13 05:39
Attached patch uses the approach described in msg135881.
msg135952 - (view) Author: Roundup Robot (python-dev) Date: 2011-05-14 03:53
New changeset 5b0fecd2eba0 by Ezio Melotti in branch '2.7':
#5723: Improve json tests to be executed with and without accelerations.
http://hg.python.org/cpython/rev/5b0fecd2eba0

New changeset c2853a54b29e by Ezio Melotti in branch '3.1':
#5723: Improve json tests to be executed with and without accelerations.
http://hg.python.org/cpython/rev/c2853a54b29e

New changeset 63fb2b811c9d by Ezio Melotti in branch '3.2':
#5723: merge with 3.1.
http://hg.python.org/cpython/rev/63fb2b811c9d

New changeset afdc06f2552f by Ezio Melotti in branch 'default':
#5723: merge with 3.2.
http://hg.python.org/cpython/rev/afdc06f2552f
History
Date User Action Args
2011-05-14 03:55:31ezio.melottisetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2011-05-14 03:53:17python-devsetnosy: + python-dev
messages: + msg135952
2011-05-13 05:39:55ezio.melottisetfiles: + issue5723-2.diff

messages: + msg135889
2011-05-12 23:32:42ezio.melottisetmessages: + msg135884
2011-05-12 23:23:51r.david.murraysetnosy: + r.david.murray
messages: + msg135883
2011-05-12 22:47:45pitrousetmessages: + msg135881
2011-05-12 22:35:35ezio.melottisetmessages: + msg135880
2011-05-12 21:56:27pitrousetmessages: + msg135872
2011-05-12 20:02:42ezio.melottisetmessages: + msg135858
2011-05-12 11:31:53pitrousetmessages: + msg135826
2011-05-12 05:45:16ezio.melottisetfiles: + issue5723.diff
versions: + Python 3.2, Python 3.3, - Python 2.6
messages: + msg135817

assignee: bob.ippolito -> ezio.melotti
keywords: + needs review, patch
stage: test needed -> commit review
2011-04-26 15:03:32xuanjisetnosy: + xuanji
2011-04-17 17:01:30bob.ippolitosetmessages: + msg133932
2011-04-13 07:18:08ezio.melottisetmessages: + msg133644
2010-08-05 00:33:52jowilliasetnosy: + jowillia
2010-07-12 15:49:54ezio.melottisetnosy: + ezio.melotti
2010-07-12 15:48:41fdrakesetnosy: + fdrake

messages: + msg110101
versions: + Python 2.6
2009-04-11 09:09:38pitrousetmessages: + msg85843
2009-04-10 00:50:30bob.ippolitosetmessages: + msg85837
2009-04-08 16:07:54rhettingersetnosy: + rhettinger
messages: + msg85774
2009-04-08 16:04:47doerwaltersetnosy: + doerwalter
messages: + msg85773
2009-04-08 16:01:34pitrousetmessages: + msg85772
2009-04-08 15:51:43bob.ippolitosetmessages: + msg85771
2009-04-08 15:43:23pitroucreate