> 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
> 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?
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. 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.
IMO, each test should be run in the same environment if it is executed
alone or as a part of a test suite. 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.
> 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
