Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#12211 closed enhancement (fixed)

Base test case classes with helper methods

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.11
Component: general Version:
Severity: normal Keywords: test
Cc: Branch:
Release Notes:

Refactored test cases to use MockRequest.

API Changes:

Added MockRequest class to trac.test.

Internal Changes:

Description

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.

Attachments (0)

Change History (30)

comment:1 by Ryan J Ollos, 5 years ago

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.

in reply to:  1 comment:2 by Ryan J Ollos, 5 years ago

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.

comment:3 by Ryan J Ollos, 5 years ago

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.

comment:4 by Ryan J Ollos, 5 years ago

comment:3 changes committed in [14535].

comment:5 by Ryan J Ollos, 5 years ago

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

comment:6 by Christian Boos, 5 years ago

Very nice indeed (both TicketTestCase and MockRequest).

comment:7 by Ryan J Ollos, 5 years ago

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.

comment:8 by Ryan J Ollos, 5 years ago

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

comment:9 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)
Milestone: next-major-releases1.0.11
Owner: set to Ryan J Ollos
Release Notes: modified (diff)
Status: newassigned

comment:10 by Ryan J Ollos, 5 years ago

Resolution: fixed
Status: assignedclosed

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

comment:11 by Jun Omae, 5 years ago

Resolution: fixed
Status: closedreopened

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.

Last edited 5 years ago by Jun Omae (previous) (diff)

comment:12 by Ryan J Ollos, 5 years ago

Resolution: fixed
Status: reopenedclosed

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

comment:13 by Ryan J Ollos, 5 years ago

Additional changes in [14659,14660].

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:14 by Ryan J Ollos, 5 years ago

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

comment:15 by Ryan J Ollos, 4 years ago

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

comment:16 by Ryan J Ollos, 4 years ago

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

in reply to:  description comment:17 by Ryan J Ollos, 4 years ago

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.

comment:18 by Ryan J Ollos, 3 years ago

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

    diff --git a/trac/test.py b/trac/test.py
    index ed3f1e3d3..381565109 100755
    a b def MockRequest(env, **kwargs):  
    155155
    156156    if 'arg_list' in kwargs:
    157157        arg_list = kwargs['arg_list']
    158         args = arg_list_to_args(arg_list)
     158        args = arg_list_to_args([(k, unicode(v)) for k, v in arg_list])
    159159    else:
    160160        args = _RequestArgs()
    161         args.update(kwargs.get('args', {}))
     161        args.update({k: unicode(v)
     162                     for k, v in kwargs.get('args', {}).iteritems()})
    162163        arg_list = [(name, value) for name in args
    163164                                  for value in args.getlist(name)]

comment:19 by Jun Omae, 3 years ago

Converting to string sounds good.

comment:20 by Ryan J Ollos, 3 years ago

Thanks. Minor change to proposed patch:

  • trac/test.py

    commit ab5504de15f2019b36d0af909d0dff93b909cd06
    Author: Ryan J Ollos <ryan.j.ollos@gmail.com>
    Date:   Tue Apr 17 15:05:14 2018 -0700
    
        Convert args to string
    
    diff --git a/trac/test.py b/trac/test.py
    index ed3f1e3d3..5d82f7eed 100755
    a b def MockRequest(env, **kwargs):  
    154154        perm = PermissionCache(env, authname)
    155155
    156156    if 'arg_list' in kwargs:
    157         arg_list = kwargs['arg_list']
     157        arg_list = [(k, unicode(v)) for k, v in kwargs['arg_list']]
    158158        args = arg_list_to_args(arg_list)
    159159    else:
    160160        args = _RequestArgs()
    161         args.update(kwargs.get('args', {}))
     161        args.update({k: unicode(v)
     162                     for k, v in kwargs.get('args', {}).iteritems()})
    162163        arg_list = [(name, value) for name in args
    163164                                  for value in args.getlist(name)]
    164165

comment:21 by Ryan J Ollos, 3 years ago

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.

comment:22 by Jun Omae, 3 years ago

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

in reply to:  22 comment:23 by Jun Omae, 3 years ago

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

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

in reply to:  22 comment:24 by Ryan J Ollos, 3 years ago

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.

comment:25 by Ryan J Ollos, 3 years ago

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

  • trac/test.py

    commit 92ce4268ea52d2a7685c6ab887bdf42703450ea8
    Author: Ryan J Ollos <ryan.j.ollos@gmail.com>
    Date:   Thu Apr 19 15:12:40 2018 -0700
    
        Convert args to string
    
    diff --git a/trac/test.py b/trac/test.py
    index 97df0c345..61e6c6070 100755
    a b def MockRequest(env, **kwargs):  
    153153    else:
    154154        perm = PermissionCache(env, authname)
    155155
     156    def convert(val):
     157        if isinstance(val, bool):
     158            return unicode(int(val))
     159        elif isinstance(val, (list, tuple)):
     160            return [unicode(v) for v in val]
     161        else:
     162            return unicode(val)
     163
    156164    if 'arg_list' in kwargs:
    157         arg_list = [(k, unicode(v)) for k, v in kwargs['arg_list']]
     165        arg_list = [(k, convert(v)) for k, v in kwargs['arg_list']]
    158166        args = arg_list_to_args(arg_list)
    159167    else:
    160168        args = _RequestArgs()
    161         args.update((k, unicode(v))
     169        args.update((k, convert(v))
    162170                    for k, v in kwargs.get('args', {}).iteritems())
    163171        arg_list = [(name, value) for name in args
    164172                                  for value in args.getlist(name)]

comment:26 by Ryan J Ollos, 3 years ago

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

comment:27 by Jun Omae, 3 years ago

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

    diff --git a/trac/test.py b/trac/test.py
    index 001d45107..d5b297d70 100755
    a b  
    2020from __future__ import with_statement
    2121
    2222import doctest
    23 import numbers
    2423import os
    2524import shutil
    2625import sys
    from trac.web.api import _RequestArgs, Request, arg_list_to_args  
    4948from trac.web.session import Session
    5049
    5150
     51try:
     52    from numbers import Real as number_type
     53except ImportError:
     54    number_type = (int, long, float)
     55
     56
    5257def Mock(bases=(), *initargs, **kw):
    5358    """
    5459    Simple factory for dummy classes that can be used as replacement for the
    def MockRequest(env, **kwargs):  
    157162    def convert(val):
    158163        if isinstance(val, bool):
    159164            return unicode(int(val))
    160         elif isinstance(val, numbers.Number):
     165        elif isinstance(val, number_type):
    161166            return unicode(val)
    162167        elif isinstance(val, (list, tuple)):
    163168            return [convert(v) for v in val]
Last edited 3 years ago by Jun Omae (previous) (diff)

comment:28 by Ryan J Ollos, 3 years ago

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

comment:29 by Ryan J Ollos, 3 years ago

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

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

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:30 by Ryan J Ollos, 3 years ago

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.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.