On 5/22/13, Anze Staric <[email protected]> wrote:
>> the question I'd rather ask is why is it compulsory to monkey patch
>> ConnectionPool methods ?
>
> It is not, but as I see it ti solves more problems that is causes.
> Beside passing the tests that set config values in __init__, ti also
> enables us to replace
> try:
> self.mpsystem.upgrade_environment(env.db_transaction)
> except OperationalError:
> # table remains but database version is deleted
> pass
>
I'm just concerned about two facts
- in production changes introduced by monkey patching that class
will not be activated so , in way , we might be cheating ourselves
if this has further implications somewhere else ...
- ... mainly because those changes are not bound to the specific
test cases that might require such modifications ,
but the whole test suite
>> In general , the more we monkey patch real classes to make test cases
>> pass , we'll be less confident of test results because such
>> modifications will not be available on production and might have
>> non-trivial side-effects at testing time obscuring behaviors that
>> might happen in practice . I'd rather prefer to (always | sometimes)
>> replace them with mock objects (e.g. EnvironmentStub , Mock ...)
>
> If we want to test multiproduct functionality, we need to upgrade the
> database schema (add product column, ...) When should this be done?
There are multiple valid answers to this questions :
- unit tests are lightweight enough (e.g. in-memory & in-process DB)
that EnvironmentStub(s) are set up and torn down once per test case .
The upgrade goes there
- functional tests are heavy so approach sketched above is not
recommended in practice .
Functional environment is always initialized once
(including MP upgrade ;) . In practice , I've not bothered too much
about collecting all garbage generated by previous test cases .
Instead I'd suggest to write test cases in a way that ignores changes
made elsewhere . Beyond that, there are two options to access
global test environment :
* Global var (like in rpctestenv in RPC plugin test suite)
* Trac built-in fixtures
> Currently, each call to setUp tries to upgrade schema using the
> following code:
> try:
> self.mpsystem.upgrade_environment(env.db_transaction)
> except OperationalError:
> # table remains but database version is deleted
> pass
> which fails, if the database is shared between tests and has been
> upgraded already.
For unit tests , if it's reset in tearDown method then next setUp will
start from scratch i.e. no reason to fail unless something is wrong
... but this is just theory , reality supported by facts may point to
other directions ;) ... the truth is in the details .
If sub-classing MultiproductTestcase there are helpers methods for that .
> A side effect of this code is, that the first time
> it is called, default product is inserted to the database, but for the
> subsequent calls, it is not.
>
subsequent calls of ... ?
> IMO, each test should be run in the same environment if it is executed
> alone or as a part of a test suite.
That depends on how you write the tests . What I advocate is :
- Test case environment should satisfy the same pre-conditions
(e.g. multi-product upgraded with default product) .
* ... and should be as close to the real system as possible ,
otherwise the test suite can not be trusted since it's testing
something else , not the system
- What is really important is that processing taking place in test case
will not have side-effects influencing the results of subsequent TCs ;
otherwise the test suite starts to be useless (that's what reset_db
in tearDown is for, to start next TC from scratch)
> If we need to choose between a 3
> line monkey patch for ConnectionPool to allow each test to run in a
> separate in-memory databases and manually reproducing the side effects
> of multiproduct upgrade, product environment configuration, and who
> know what have we missed, in test setUp, I vote for the monkey patch.
>
FWIW , it's worthless to adopt a shortcut if the test environment is
different enough that test results can not be trusted . The goal
resides in being able to make judgments on the correct behavior of the
system by reviewing test results .
Q:
- How will this change impact upon other test cases ?
- Is this closer to the code run in production ?
- ... if not , is there a chance for this to obscure failure modes
in production ?
- ... or introduce false negatives in test reports ?
>> In general , the more we monkey patch real classes to make test cases
>> pass , we'll be less confident of test results because such
>> modifications will not be available on production and might have
>> non-trivial side-effects at testing time obscuring behaviors that
>> might happen in practice. I'd rather prefer to (always | sometimes)
>> replace them with mock objects (e.g. EnvironmentStub , Mock ...)
>
> I could also replace ConnectionPoll with a custom class, that overrode
> this function, but I would still need to inject it to trac.db.pool as
> DatabaseManager has no way of replacing the implementation of
> ConnectionPool. When faced with these two options, I prefer monkey
> patches
>
If really needed I'd rather advocate to monkey patch that class in
setUp method of test cases really in need of this modification , and
revert changes in tearDown method .
--
Regards,
Olemis.
Apache™ Bloodhound contributor
http://issues.apache.org/bloodhound
Blog ES: http://simelo-es.blogspot.com/
Blog EN: http://simelo-en.blogspot.com/
Featured article:
