Edward d'Auvergne wrote:
On 11/21/06, Gary S. Thompson <garyt@xxxxxxxxxxxxxxx> wrote:
Edward d'Auvergne wrote:
> That looks awesome, thank you for porting the code into the
> conventions used in relax. The changes will significantly help with
> the API docs, internal help system, etc. and will help standardise
> these docs. I have to apologise for the tediousness of these
> standards - I realise this is not what you would normally expect from
> source code. The reason for these conventions is that I'm trying to
> design the relax source code to be highly readable to allow NMR
> spectroscopists, most of which are either not programmers or have just
> a little programming experience, to be able to easily understand the
> code and extend it's capabilities. The point is to lower the barrier
> of entry into relax so that non-programmers can read the code and
> mimic it to add features that they find useful. Because this is a
> true open source project, many people will eventually read the code.
> The number of people looking at the code is amplified by the fact the
> users are NMR spectroscopists who are generally not afraid to dive
> into code to find problems.
>
Thats fine and in fact a lot of opensource projects have quite strict
conventions on layout. Though most don't expect as much commenting ;-)
But the reasons for that are fair enough here (though there can be
problems with comments and code getting out of sync, which is why most
programmer prefer clear code to comments ;-))
It's not too hard to keep the comments in sync, especially when the
code tells you how while the comments tell you why :)
> Once integrated into
> relax, I think the 'get_startup_path()' can be dropped as
> 'sys.path[-1]' will give the same result.
I would like to avaoid that. The framework is designed so that you will
be able to run parts of the relax unit tests without invoking relax...
(i.e. it will have a 'main' function) and the only available convention
to identify the startup directory is to look at sys.path[0]. The other
thing to say here is that I guess that sys.path[0] ought to stay as the
startup directory as this is the python convention (is there a pressing
need to remove the relax path from the pythonpath?) Also
get_startup_path() also allows for the case when sys.path[0] == ''
which implies the current working directory. Finally if relax does need
to put something before the startup path I guess the other alternative
would be to salt the original path away somewhere else and pass it to
test_runner and thence to test_finder. I could then remove the
dependancy between test_finder and 'get_startup_path()
I've already added the ability to run the system/functional tests
followed by the unit tests through the '--test-suite' command line
option (in the 'test_suite' branch). It shouldn't hurt to allow both
methods of running the tests. For the system/functional tests having
a running instance of relax, either controlling or controlled by the
test suite, is essential anyway.
As for 'sys.path[-1]' being the relax base directory and 'sys.path[0]'
being '.', this is currently only being used by the system/functional
tests to access the path in which the test data is located. I just
looked back at the history and this is some of the most ancient code
in relax - it was introduced in relax version 0.1 at r69 back on Dec
21st, 2001!!! I'm pretty sure this is not necessary any more!
If this code is removed, then all instances of 'sys.path[-1]' could be
replaced by 'sys.path[0]'. To allow the unit tests to be run as part
of the relax test suite or separately, the function
'get_startup_path()' can't hurt. I'll take back everything I said
previously.
> Although I don't have the
> time to do this yet (I will find the time in the future), many of
> these functions could be utilised by the system/function tests. In
> the future they could possibly be shifted into a base class used by
> both the unit and system/function tests.
This was my intention (chris and I talked quite alot about the best way
to do this) and one of the next things on my list was to migrate one of
the system test to use the 'unit test' framework. The reason the
unit_test_runner is where it is is so that I have some unit tests to
hand for testing I am intending to move it further up the hierachy.
I have started to look into using the unittest framework for the
system/functional tests. I do envisage a different layout than the
unit tests though. For instance I'm wondering if I'll need to code a
replacement or subclass the TextTestRunner class.
I think you will need a subclass of test runner to do these things
Do you know if
TextTestRunner catches both stdout and stderr? The relax printout
messages need to be suppressed when a test passes. If a failure does
occur, the error object contains the traceback but I would like to
have the normal program printout presented followed by the traceback
for debugging purposes.
I would like a clear distinction between the different classes of
tests so that relax can generate a report in the end. I.e. all the
unit tests are clustered into a single TestSuite (which can be
composed of multiple TestSuite objects for grouping) and all the
system/functional tests clustered into a single separate TestSuite
object. Although the system/functional tests could possibly use the
unittest framework, the unit tests and system tests don't have to be
within the same framework.
No indeed and this is one of the thoughts we had. The TestSuite tree of
instances should be something like this
system_tests
|
--------------------------------------------
| | | |
model_selection jw_mapping ... unit_tests
|
-----------------------------
| | |
test_x test_y ...
(if the tree doesn't display you need to use a monospaced font)
I envisage the system tests to work quite differently in that the
instance 'self.relax' is passed into each test class (this is
essential). Most of the automatic stuff in the unit tests will be
unnecessary. For example functions such as
'get_module_relative_path()', etc. will be unnecessary.
Indeed this is true and my plan is to create a super class which just
did test discovery without having to find relative paths etc the root
path would just be provided
(We talked about subclassing TestCase as well so it has an idea of it's
directory and the root test_suite directory to give tests places to find
data from)
Actually on
the subject of these functions, the ones not defined as class methods,
they could all be shifted into the classes and accessed by typing
'self.get_module_relative_path()'. Apart from the print statements at
the end of 'unit_test_runner.py', these functions are only accessed by
the other class methods. These may as well be converted to an object
oriented approach.
Now that is an interesting question: Should they go in the class? Most
probably. However, should they be virtual methods that can be overidden?
Almost definitley not in my opinion (overridden methods should always be
designed to be so) So I suggest they go in as static methods of the
class [this was my plan anyway I just have to get down to trying it]
also most of these methods should be private [they are not part of the
public interface])
This leads to another question what is the minimum python version that
we are aiming for in the 1.3 branch?
(i.e. what facilities are available to us)
> I'll look into this once I
> attempt to migrate the system/function tests to use parts or all of
> the unittest framework. At the moment though, we could simply have
> relax run the unit tests followed by the system/function tests (or the
> other way around).
The intention was to have a functional test that ran the unit_tester ;-)
It could be implemented that way. It may be better to keep a
separation between the different types of test though, especially
considering how big the unit tests may become. Then relax can present
a report of each category, and only pass if all categories pass. It
will allow more categories of tests to be added separately - for
example installation tests, compilation tests, etc.
My thoughts are that unit tests should be a category in the test suite.
Certainly running relax with a command line flag such as -test or
-test_suite should run all the tests available as a default (because
this is what the user generally wants). If we need the ability to test
sub sets of tests then we need some extra options (philosophically we
should be aiming for users testers and in fact everyone to run all the
tests by default, we catch more bugs that way...)
Two other questions
1. do you use the '_'function_name convention for private functions
Currently relax doesn't have private functions - I don't see a need
for this,
Actually I see quite a strong impetus for private methods/classes in
code that is used/developed by more than one person. In general a class
should define a public interface (bits shat shouldn't go away) and
private methods (implimentation details)...
except of course for the eventual conversion of 'float.py'
into a separate Python module. I have used the 'self._xxxx()' style
only in the automatic diffusion tensor object creation code and this
only hides these functions by a search statement within the
'self.__repr__()' methods of the relax data structure
'self.relax.data'. I know Python fudges private functions using the
'self.__xxxx()' notation, but I have a feeling site-packages do this
differently, not through private objects but by other methods. I
could be wrong though.
Actually I am pretty sure you are right. Though if you ask guido he
would tell you that the __ thing isn't a fudge it is just a marker
(almost all public/private systems can be compromised by a determined
programmer so why bother with anything other than information?)
Further points. My intention wa to rename all the system tests so they
were of the form test_<function>.py so that we can use the unit test
automatic discovery and then rewrite main.py to use a TestRunner and
TestCase setup (headings etc would then come from instance variables) I
can try and create something that does what is already in parallel using
the new infrastructure (maybe we should leave the test suite branch up
for the moment?)
2. I like the relax code style checker
(http://nmr-relax.com/scripts/code_validator). However, it seems to
produce an annoying number of false positives, is this something that is
being worked on? (it would be really nice to be able to run it and see
no complaints on conforming code, it would make life much more asy for
the devloper) Also I have a few pathological cases where it fails should
i foward those as some sort of bug report?
I guess it would have to be a fairly major overhaul (I would think using
the python ast classes would be a good idea). So maybe sometime ;-)
The repository of web pages have now been moved into the subversion
repository. It's in the directory 'website' viewable at
http://svn.gna.org/viewcvs/relax/. This 'code_validator' script was
an old script I used for cleaning up relax source code that I have
slightly extended. It has always reported many false positives but if
you would like to improve this script to avoid these false results, go
for it! There's probably not much use creating a bug report. Also,
each thing it checks has different levels of importance. Some parts
are essential, others are cosmetic, and one or two are highly
superficial. I am eventually planning on categorising these checks.
Cheers,
Edward
.
--
-------------------------------------------------------------------
Dr Gary Thompson
Astbury Centre for Structural Molecular Biology,
University of Leeds, Astbury Building,
Leeds, LS2 9JT, West-Yorkshire, UK Tel. +44-113-3433024
email: garyt@xxxxxxxxxxxxxxx Fax +44-113-2331407
-------------------------------------------------------------------
Re: r2860 - /branches/test_suite/test_suite/unit_tests/unit_test_runner.py