Code review comment for lp:~lifeless/launchpad/doctest

On Tue, Oct 12, 2010 at 6:43 AM, Robert Collins
<email address hidden> wrote:
> Robert Collins has proposed merging lp:~lifeless/launchpad/doctest into lp:launchpad/devel.
>
> Requested reviews:
>  Launchpad code reviewers (launchpad-reviewers)
>
>
> Prep work for parallel testing. One of the main components of parallel testing is programmatic rearrangement of the tests to run, and story tests in particular were a problem.
>
> Previously the test *id* for the whole story was pagetest: relname. This is ok, but I've changed it to story- to make it more clear that this is for *stories*, not just pagetests.
>

You might be interested to know that some people insist on calling all
pagetests "stories". Nevertheless I take you to mean the silly
pagetests that must run in sequence (e.g. 01-foo.txt).

> And now the problem bit: the delegated tests were reporting their own ids; those ids don't exist (naturally, the story appearing as a single test so it can't be split hides the components) - and even if they did exist in the full test list, they can't be run separately.
>
> So this branch changes the ids of the individual doctests so that they all claim to be the story as far as subunit and testr is concerned. This makes the full test list able to be partitioned and parallel tested without skipping tests, and while we can make do when starting with a fresh test list(an unrun test list has the story ids), we need to be able to feed data from test runs back into the split algorithm so that we can take advantage of test performance data to make the partitions perform equally.
>
> The downside is that when a test fails, the test *id* won't specify the exact text file for stories (01-foo-bar.txt). OTOH we're removing those incrementally, and the filename in the detailed error page will be correct. So its not a big deal IMO.

This seems like a bearable solution.

> === modified file 'lib/canonical/launchpad/testing/pages.py'
> --- lib/canonical/launchpad/testing/pages.py    2010-10-04 20:46:55 +0000
> +++ lib/canonical/launchpad/testing/pages.py    2010-10-12 05:43:43 +0000
> @@ -848,7 +848,7 @@
>         return self._suite.countTestCases()
>
>     def shortDescription(self):
> -        return "pagetest: %s" % self._description
> +        return self._description
>
>     def id(self):
>         return self.shortDescription()
> @@ -910,20 +910,27 @@
>     numberedfilenames = sorted(numberedfilenames)
>     unnumberedfilenames = sorted(unnumberedfilenames)
>
> +    suite = unittest.TestSuite()
> +    checker = SpecialOutputChecker()
>     # Add unnumbered tests to the suite individually.
> -    checker = SpecialOutputChecker()
> -    suite = LayeredDocFileSuite(
> -        package=package, checker=checker, stdout_logging=False,
> -        layer=PageTestLayer, setUp=setUp,
> -        *[os.path.join(storydir, filename)
> -          for filename in unnumberedfilenames])
> +    if unnumberedfilenames:
> +        suite.addTest(LayeredDocFileSuite(
> +            package=package, checker=checker, stdout_logging=False,
> +            layer=PageTestLayer, setUp=setUp,
> +            *[os.path.join(storydir, filename)
> +              for filename in unnumberedfilenames]))
>
>     # Add numbered tests to the suite as a single story.
> -    storysuite = LayeredDocFileSuite(
> -        package=package, checker=checker, stdout_logging=False,
> -        setUp=setUp,
> -        *[os.path.join(storydir, filename)
> -          for filename in numberedfilenames])
> -    suite.addTest(PageStoryTestCase(stripped_storydir, storysuite))
> +    if numberedfilenames:
> +        storysuite = LayeredDocFileSuite(
> +            package=package, checker=checker, stdout_logging=False,
> +            setUp=setUp,
> +            *[os.path.join(storydir, filename)
> +              for filename in numberedfilenames])
> +        story_test_id = "story-%s" % stripped_storydir
> +        get_id = lambda: story_test_id
> +        for test in storysuite:
> +            test.id = get_id
> +        suite.addTest(PageStoryTestCase(story_test_id, storysuite))
>

This deserves a comment, to say why you're doing it.

Please add the comment and land.

Thanks for the patch!
jml