Base test case classes with helper methods

[Ryan J Ollos]

I've been considering ways to reduce the code duplication of helper methods in the test code. For example, there are at least a half-dozen instances of _insert_ticket and _create_request. One possibility would be to create a mixin-like base class with the helper methods.

[Ryan J Ollos]

Example of changes I have in mind can be found in log:rjollos.git:t12211_base_test_classes. I'm very interested to know if someone sees a better way to do this.

[Ryan J Ollos]

Replying to Ryan J Ollos:

I'm very interested to know if someone sees a better way to do this.

I've considered a few alternative approaches:

1. Create helper functions in trac.test.
2. Create helper class(es) in trac.test. The classes could be instantiated as needed in test cases. This would minimize the number of methods relative to approach (1).
3. Attach the helper functions to trac.test.EnvironmentStub. One downside is that the number of methods on the class could grow large very quickly.

My main aim for this ticket is to find an approach that avoids defining _create_request on numerous TestCases.

[Ryan J Ollos]

Replaced uses of _insert_user with EnvironmentStub.insert_known_users in [14524]. Additional proposed changes in log:rjollos.git:t12211_known_users. insert_known_users was added in 1.1.x, so it wouldn't be a breaking API change to rename the method.

Continuing the thought in comment:2, it might make sense to define insert_users on a class other than EnvironmentStub.

[Ryan J Ollos]

comment:3 changes committed in [14535].

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t12211_mock_request. I'll add class documentation before committing.

[Christian Boos]

Very nice indeed (both TicketTestCase and MockRequest).

[Ryan J Ollos]

Thanks, I added TicketTestCase to the changes ([9763655/rjollos.git]) and I'll spend more time reviewing the code in trac.ticket.tests to see what other methods can be added to the class.

[Ryan J Ollos]

I modified MockRequest to use Request as a base class and populate the callbacks: log:rjollos.git:t12211_mock_request.1.

[Ryan J Ollos]

Committed to 1.0-stable in [14641], merged to trunk in [14642].

[Jun Omae]

After [14635], a TypeError is raised when accessing project index page.

Exception happened during processing of request from ('192.168.11.17', 58881)
Traceback (most recent call last):
  File "/usr/lib/python2.6/SocketServer.py", line 560, in process_request_thread
    self.finish_request(request, client_address)
  File "/usr/lib/python2.6/SocketServer.py", line 322, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/lib/python2.6/SocketServer.py", line 617, in __init__
    self.handle()
  File "/usr/lib/python2.6/BaseHTTPServer.py", line 329, in handle
    self.handle_one_request()
  File "/home/jun66j5/src/tracdev/git/trac/web/wsgi.py", line 192, in handle_one_request
    gateway.run(self.server.application)
  File "/home/jun66j5/src/tracdev/git/trac/web/wsgi.py", line 92, in run
    response = application(self.environ, self._start_response)
  File "trac/web/standalone.py", line 59, in __call__
    return self.application(environ, start_response)
  File "trac/web/standalone.py", line 91, in __call__
    return self.application(environ, start_response)
  File "/home/jun66j5/src/tracdev/git/trac/web/main.py", line 461, in dispatch_request
    env_paths)
  File "/home/jun66j5/src/tracdev/git/trac/web/main.py", line 704, in send_project_index
    'time': user_time(req, format_datetime)},
  File "/home/jun66j5/src/tracdev/git/trac/util/datefmt.py", line 874, in user_time
    kwargs['tzinfo'] = getattr(req, 'tz', None)
  File "/home/jun66j5/src/tracdev/git/trac/web/api.py", line 370, in __getattr__
    value = self.callbacks[name](self)
TypeError: 'NoneType' object is not callable

We don't set None to Request.callbacks. That must be a callable instance.

[Ryan J Ollos]

Partial revert of [14635] in [14655], merged to trunk in [14656].

[Ryan J Ollos]

Additional changes in [14659,14660].

[Ryan J Ollos]

Replacement of Mock with MockRequest in [14666,14667,14668]. PEP8 changes in [14669].

[Ryan J Ollos]

Replaced DetachedSession with Session in r15047, merged in r15048, r15049.

[Ryan J Ollos]

Additional refactoring on 1.2-stable in r15051, merged to trunk in r15052.

[Ryan J Ollos]

Replying to Ryan J Ollos:

I've been considering ways to reduce the code duplication of helper methods in the test code. For example, there are at least a half-dozen instances of _insert_ticket and _create_request. One possibility would be to create a mixin-like base class with the helper methods.

_insert_ticket will be addressed in #12812.

[Ryan J Ollos]

Someone might pass a non-string value in args like {'version': 1}. I propose that we convert all values in the args dict to string:

trac/test.py

@@ -155 +155 @@
 
     if 'arg_list' in kwargs:
         arg_list = kwargs['arg_list']
-        args = arg_list_to_args(arg_list)
+        args = arg_list_to_args([(k, unicode(v)) for k, v in arg_list])
     else:
         args = _RequestArgs()
-        args.update(kwargs.get('args', {}))
+        args.update({k: unicode(v)
+                     for k, v in kwargs.get('args', {}).iteritems()})
         arg_list = [(name, value) for name in args
                                   for value in args.getlist(name)]

[Jun Omae]

Converting to string sounds good.

[Ryan J Ollos]

Thanks. Minor change to proposed patch:

trac/test.py

@@ -154 +154 @@
         perm = PermissionCache(env, authname)
 
     if 'arg_list' in kwargs:
-        arg_list = kwargs['arg_list']
+        arg_list = [(k, unicode(v)) for k, v in kwargs['arg_list']]
         args = arg_list_to_args(arg_list)
     else:
         args = _RequestArgs()
-        args.update(kwargs.get('args', {}))
+        args.update({k: unicode(v)
+                     for k, v in kwargs.get('args', {}).iteritems()})
         arg_list = [(name, value) for name in args
                                   for value in args.getlist(name)]
 

[Ryan J Ollos]

comment:20 change committed to 1.0-stable in r16498, but there's a problem with the patch. The argument value could be list type​, and several tests pass a boolean value​. I'll create another patch to handle those cases.

[Jun Omae]

Travis CI build failed at ​https://travis-ci.org/edgewall/trac/builds/368426764 but I didn't receive notification mail.

Anyaway, r16498 leads syntax errors on Python 2.5 and 2.6.

...
python ./trac/test.py --skip-functional-tests 
  File "./trac/test.py", line 162
    for k, v in kwargs.get('args', {}).iteritems()})
      ^
SyntaxError: invalid syntax
make: *** [unit-test] Error 1

[Jun Omae]

Anyaway, r16498 leads syntax errors on Python 2.5 and 2.6.

Fixed in r16501 using an iterable of key/value pairs.

[Ryan J Ollos]

Replying to Jun Omae:

Travis CI build failed at ​https://travis-ci.org/edgewall/trac/builds/368426764 but I didn't receive notification mail.

A few months back the notifications were delayed for some time (comment:12:ticket:12839). Last notification from trac-builds@ was April 6th. I might suspect the change in comment:13:ticket:12839, but that was working for more than a month.

[Ryan J Ollos]

Thanks for comment:23 fix. Here is a patch to handle cases described in comment:21:

trac/test.py

@@ -153 +153 @@
     else:
         perm = PermissionCache(env, authname)
 
+    def convert(val):
+        if isinstance(val, bool):
+            return unicode(int(val))
+        elif isinstance(val, (list, tuple)):
+            return [unicode(v) for v in val]
+        else:
+            return unicode(val)
+
     if 'arg_list' in kwargs:
-        arg_list = [(k, unicode(v)) for k, v in kwargs['arg_list']]
+        arg_list = [(k, convert(v)) for k, v in kwargs['arg_list']]
         args = arg_list_to_args(arg_list)
     else:
         args = _RequestArgs()
-        args.update((k, unicode(v))
+        args.update((k, convert(v))
                     for k, v in kwargs.get('args', {}).iteritems())
         arg_list = [(name, value) for name in args
                                   for value in args.getlist(name)]

[Ryan J Ollos]

comment:25 and additional change committed to 1.0-stable in [16525,16527], merged in [16526,16528], r16530.

[Jun Omae]

In [16527], numbers module is not available on Python 2.5.

$ python2.5
Python 2.5.6 (r256:88840, Oct 21 2014, 22:49:55)
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import numbers
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named numbers

At least, I think we should use numbers.Real rather than numbers.Number because complex number is a numbers.Number (use numbers.Integral if float is not expected).

Python 2.6.9 (default, Oct 22 2014, 19:56:52)
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import numbers
>>> isinstance(complex(1, 2), numbers.Number)
True

trac/test.py

@@ -20 +20 @@
 from __future__ import with_statement
 
 import doctest
-import numbers
 import os
 import shutil
 import sys
@@ -49 +48 @@
 from trac.web.session import Session
 
 
+try:
+    from numbers import Real as number_type
+except ImportError:
+    number_type = (int, long, float)
+
+
 def Mock(bases=(), *initargs, **kw):
     """
     Simple factory for dummy classes that can be used as replacement for the
@@ -157 +162 @@
     def convert(val):
         if isinstance(val, bool):
             return unicode(int(val))
-        elif isinstance(val, numbers.Number):
+        elif isinstance(val, number_type):
             return unicode(val)
         elif isinstance(val, (list, tuple)):
             return [convert(v) for v in val]

[Ryan J Ollos]

comment:27 patch applied in r16539, merged in r16540, r16541.

[Ryan J Ollos]

Minor revision of r14641 in r16648, merged in r16649, r16650.

Fixed error in r14641 in r16653, merged in r16654, r16655.

[Ryan J Ollos]

If I was implementing MockRequest now, I would add a property response_sent that returned the value from the buffer, to avoid accessing as req.response_sent.getvalue(). See, for example, what is done here: trunk/trac/web/tests/api.py@16656:106-108#L88​. However, Request.response_sent is an attribute and changing it to a property would be an API change.

I suppose we could add an accessor such as get_response_sent(), but I tend to think it is confusing to developers when we have multiple public attributes and functions for accessing the same data.