[MERGE] repository external reference tests (StackedBranches)
Vincent Ladeuil
v.ladeuil+lp at free.fr
Thu Jun 5 08:02:27 BST 2008
>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at canonical.com> writes:
Ian> Number 2 in the series of patches for stacked branches.
Ian> Robert put this up for review earlier and it was reviewed
Ian> by Andrew and Martin. I believe some of the requested tweaks
Ian> still need to be done (though I'm guessing Andrew could
Ian> do those much faster/safer than I given his knowledge of
Ian> exactly where the APIs in question are up to in their
Ian> evolution). I have made some changes though to Robert's
Ian> code to merge in vila's preferred new way of loading tests
Ian> faster.
Ian> Given the above, I'm not actually going to review this code
Ian> myself. Could Andrew or Martin re-review this please?
A couple of comments below.
Ian> Ian C.
Ian> # Bazaar merge directive format 2 (Bazaar 0.90)
Ian> # revision_id: ian.clatworthy at canonical.com-20080603065038-\
Ian> # k68rlyw2m4uw164z
Ian> # target_branch: ../HTTPServer.chdir-isolation/
Ian> # testament_sha1: 7c2bfd0d3487494693a74563d2a866b655e3859f
Ian> # timestamp: 2008-06-05 11:23:49 +1000
Ian> # base_revision_id: ian.clatworthy at canonical.com-20080603063138-\
Ian> # tu2p0z0ch3v4zcai
Ian> #
Ian> # Begin patch
Ian> === modified file 'NEWS'
Ian> --- NEWS 2008-06-03 06:31:38 +0000
Ian> +++ NEWS 2008-06-03 06:50:38 +0000
Ian> @@ -121,12 +121,15 @@
Ian> TESTING:
Ian> + * ``bzrlib.tests.adapt_tests`` was broken and unused - it has been fixed.
Ian> + (Robert Collins)
Ian> +
Ian> + * Fix the test HTTPServer to be isolated from chdir calls made while it is
Ian> + running, allowing it to be used in blackbox tests. (Robert Collins)
Ian> +
Ian> * New helper function for splitting test suites
Ian> ``split_suite_by_condition``. (Robert Collins)
Ian> - * Fix the test HTTPServer to be isolated from chdir calls made while it is
Ian> - running, allowing it to be used in blackbox tests. (Robert Collins)
Ian> -
Ian> INTERNALS:
Ian> * ``Branch.missing_revisions`` has been deprecated. Similar functionality
Ian> === modified file 'bzrlib/tests/__init__.py'
Ian> --- bzrlib/tests/__init__.py 2008-05-12 10:16:36 +0000
Ian> +++ bzrlib/tests/__init__.py 2008-06-03 01:37:11 +0000
Ian> @@ -2744,6 +2744,7 @@
Ian> 'bzrlib.tests.test_registry',
Ian> 'bzrlib.tests.test_remote',
Ian> 'bzrlib.tests.test_repository',
Ian> + 'bzrlib.tests.repository_external_reference_implementations',
Ian> 'bzrlib.tests.test_revert',
Ian> 'bzrlib.tests.test_revision',
Ian> 'bzrlib.tests.test_revisionspec',
Ian> @@ -2964,16 +2965,16 @@
Ian> def adapt_modules(mods_list, adapter, loader, suite):
Ian> """Adapt the modules in mods_list using adapter and add to suite."""
Ian> - for test in iter_suite_tests(loader.loadTestsFromModuleNames(mods_list)):
Ian> + tests = loader.loadTestsFromModuleNames(mods_list)
Ian> + adapt_tests(tests, adapter, suite)
Innocuous here, but using module names (tests) as variables names
is always risky...
Ian> +
Ian> +
Ian> +def adapt_tests(tests_list, adapter, suite):
Ian> + """Adapt the tests in tests_list using adapter and add to suite."""
Ian> + for test in iter_suite_tests(tests_list):
Ian> suite.addTests(adapter.adapt(test))
I don't remember the details, but these helpers are not the best
part of tests/__init__.py, better leave them alone, they are
better ways for which a bit more occurrences are needed before
triangulation can occur. Not a requirement for *that* patch
though.
Anyway, you call iter_suite_tests with a parameter suffixed _list
where it really is a suite, i.e. a tree not a flat list.
These functions are already hard to understand for newcomers,
don't trick them ;-)
<snip/>
Ian> +
Ian> +def load_tests(standard_tests, module, loader):
Ian> + # format_scenarios is all the implementations of Repository; i.e. all disk
Ian> + # formats plus RemoteRepository.
Ian> + adapter = TestScenarioApplier()
Ian> + scenarios = all_repository_format_scenarios()
Ian> + adapter.scenarios = []
Ian> + for scenario in scenarios:
Ian> + format = scenario[1]['repository_format']
Ian> + # For remote repositories, we need at least one external reference
Ian> + # capable format to test it: defer this until landing such a format.
Ian> + # if isinstance(format, remote.RemoteRepositoryFormat):
Ian> + # scenario[1]['bzrdir_format'].repository_format =
Ian> + if format.supports_external_lookups:
Ian> + adapter.scenarios.append(scenario)
You are implementing a specialized adapter inline. I'd prefer a
true class, that will allow further subclassing and make the
load_tests() function easier to read and to extend.
Have a look at tests/test_http.py for a more complex example
illustrating my points.
Ian> +
Ian> + prefix = module.__name__ + '.test_'
Ian> + test_repository_modules = [
Ian> + 'add_inventory',
Ian> + 'add_revision',
Ian> + 'add_signature_text',
Ian> + 'all_revision_ids',
Ian> + 'break_lock',
Ian> + 'check',
Ian> + ]
Ian> + module_name_list = [prefix + module_name
Ian> + for module_name in test_repository_modules]
Ian> +
Ian> + # Parameterize repository_implementations test modules by format.
Ian> + result = TestSuite()
Ian> + adapt_tests(standard_tests, adapter, result)
Ian> + adapt_modules(module_name_list, adapter, loader, result)
Ian> + return result
Hmmm, so you need to parametrize all sub modules in the same
way. Ok, never encounter the need myself but I think I will load
the tests from the modules and then parametrized them from here.
You seem to import the specialized test class in all these sub
modules already, so centralizing the parametrization too will
make the whole process more discoverable.
The alternative is of course to define a load_tests function in
each of them. Depending on the refinement you need in the
parametrization, this may not be needed now, but it could one
day.
<snip/>
Ian> === modified file 'bzrlib/tests/repository_implementations/__init__.py'
Ian> --- bzrlib/tests/repository_implementations/__init__.py 2008-05-19 18:56:45 +0000
Ian> +++ bzrlib/tests/repository_implementations/__init__.py 2008-06-03 01:37:11 +0000
Ian> @@ -48,42 +48,47 @@
Ian> from bzrlib.transport.memory import MemoryServer
Ian> -class RepositoryTestProviderAdapter(TestScenarioApplier):
Ian> - """A tool to generate a suite testing multiple repository formats at once.
Ian> +def formats_to_scenarios(formats, transport_server, transport_readonly_server,
Ian> + vfs_transport_factory=None):
Ian> + """Transform the input formats to a list of scenarios.
Eerk ! You did the opposite ! Going from a class to a function
doesn't seem like the best way to allow further subclassing.
Ian> - This is done by copying the test once for each transport and injecting
Ian> - the transport_server, transport_readonly_server, and bzrdir_format and
Ian> - repository_format classes into each copy. Each copy is also given a new id()
Ian> - to make it easy to identify.
Ian> + :param formats: A list of (repository_format, bzrdir_format).
Ian> """
Ian> -
Ian> - def __init__(self, transport_server, transport_readonly_server, formats,
Ian> - vfs_transport_factory=None):
Ian> - TestScenarioApplier.__init__(self)
Ian> - self._transport_server = transport_server
Ian> - self._transport_readonly_server = transport_readonly_server
Ian> - self._vfs_transport_factory = vfs_transport_factory
Ian> - self.scenarios = self.formats_to_scenarios(formats)
Ian> -
Ian> - def formats_to_scenarios(self, formats):
Ian> - """Transform the input formats to a list of scenarios.
Ian> -
Ian> - :param formats: A list of (repository_format, bzrdir_format).
Ian> - """
Ian> - result = []
Ian> - for repository_format, bzrdir_format in formats:
Ian> - scenario = (repository_format.__class__.__name__,
Ian> - {"transport_server":self._transport_server,
Ian> - "transport_readonly_server":self._transport_readonly_server,
Ian> - "bzrdir_format":bzrdir_format,
Ian> - "repository_format":repository_format,
Ian> - })
Ian> - # Only override the test's vfs_transport_factory if one was
Ian> - # specified, otherwise just leave the default in place.
Ian> - if self._vfs_transport_factory:
Ian> - scenario[1]['vfs_transport_factory'] = self._vfs_transport_factory
Ian> - result.append(scenario)
Ian> - return result
Ian> + result = []
Ian> + for repository_format, bzrdir_format in formats:
Ian> + scenario = (repository_format.__class__.__name__,
Ian> + {"transport_server":transport_server,
Ian> + "transport_readonly_server":transport_readonly_server,
Ian> + "bzrdir_format":bzrdir_format,
Ian> + "repository_format":repository_format,
Ian> + })
Ian> + # Only override the test's vfs_transport_factory if one was
Ian> + # specified, otherwise just leave the default in place.
Ian> + if vfs_transport_factory:
Ian> + scenario[1]['vfs_transport_factory'] = vfs_transport_factory
Ian> + result.append(scenario)
Ian> + return result
Ian> +
Ian> +
Ian> +def all_repository_format_scenarios():
Ian> + """Return a list of test scenarios for parameterising repository tests."""
Ian> + registry = repository.format_registry
Ian> + all_formats = [registry.get(k) for k in registry.keys()]
Ian> + all_formats.extend(weaverepo._legacy_formats)
Ian> + # format_scenarios is all the implementations of Repository; i.e. all disk
Ian> + # formats plus RemoteRepository.
Ian> + format_scenarios = formats_to_scenarios(
Ian> + [(format, format._matchingbzrdir) for format in all_formats],
Ian> + default_transport,
Ian> + # None here will cause a readonly decorator to be created
Ian> + # by the TestCaseWithTransport.get_readonly_transport method.
Ian> + None)
Ian> + format_scenarios.extend(formats_to_scenarios(
Ian> + [(RemoteRepositoryFormat(), RemoteBzrDirFormat())],
Ian> + SmartTCPServer_for_testing,
Ian> + ReadonlySmartTCPServer_for_testing,
Ian> + MemoryServer))
Ian> + return format_scenarios
Ian> class TestCaseWithRepository(TestCaseWithBzrDir):
Ian> @@ -830,35 +835,12 @@
Ian> IncorrectlyOrderedParentsScenario,
Ian> UnreferencedFileParentsFromNoOpMergeScenario,
Ian> ]
Ian> -
Ian> +
Ian> def load_tests(basic_tests, module, loader):
Ian> result = loader.suiteClass()
Ian> # add the tests for this module
Ian> result.addTests(basic_tests)
Ian> -
Ian> - registry = repository.format_registry
Ian> - all_formats = [registry.get(k) for k in registry.keys()]
Ian> - all_formats.extend(weaverepo._legacy_formats)
Ian> - disk_format_adapter = RepositoryTestProviderAdapter(
Ian> - default_transport,
Ian> - # None here will cause a readonly decorator to be created
Ian> - # by the TestCaseWithTransport.get_readonly_transport method.
Ian> - None,
Ian> - [(format, format._matchingbzrdir) for format in all_formats])
Ian> -
Ian> - remote_repo_adapter = RepositoryTestProviderAdapter(
Ian> - SmartTCPServer_for_testing,
Ian> - ReadonlySmartTCPServer_for_testing,
Ian> - [(RemoteRepositoryFormat(), RemoteBzrDirFormat())],
Ian> - MemoryServer
Ian> - )
Ian> -
Ian> - # format_scenarios is all the implementations of Repository; i.e. all disk
Ian> - # formats plus RemoteRepository.
Ian> - format_scenarios = (disk_format_adapter.scenarios +
Ian> - remote_repo_adapter.scenarios)
Ian> -
Err, I really prefer *this* form or any form capturing the test
parameters in a single place in a single class with subclasses
tweaking the scenarios at will.
If you look at tests/test_http.py you'll see that you can then
decide test class by test class what parametrization you want to
apply.
Here you separate between all_repository_format_scenarios() and
formats_to_scenarios(), but using adapter classes leave far more
room for variations.
BB:resubmit
Vincent
More information about the bazaar
mailing list