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

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

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

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.

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.

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.

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.

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.

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.

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

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

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

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

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

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.

> 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

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?

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.

Attached patch uses the approach described in msg135881.

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