'tahoe debug trial' command

[davidsarah]

If there were a tahoe debug trial command, which runs Twisted Trial with a default testsuite of allmydata.test, that would have the following advantages:

• it would be guaranteed that the test suite run in this way would import the same code as the bin/tahoe command. Currently, setup.py trial and bin/tahoe set up sys.path in different ways, which can result in not testing the same code that would be executed [*]

• it would be possible to run tests on executables created by bb-freeze, py2exe, etc. (#585)

• the hack in setup.py#L54 to set __requires__ could be removed. Perhaps setuptools_trial would not be needed at all.

[*] The specific case in #1258 results in bin/tahoe running the "wrong" code while setup.py trial runs the "right" code, but that appears to be just a coincidence of the ordering of __requires__ lists.

[davidsarah]

src/allmydata/scripts/debug.py: add 'tahoe debug trial' command. refs #1296

[davidsarah]

Tests for 'tahoe debug trial'. refs #1296 (includes some dependency patches)

[davidsarah]

Change 'setup.py test' and 'setup.py trial' to use 'bin/tahoe debug trial'. Remove setuptools_trial which is no longer used. Remove a no-longer necessary hack from setup.py. refs #1296

[davidsarah]

Please look only at attachment:combined-debug-trial.darcs.patch​ and disregard the others.

[davidsarah]

Combined changes to support 'tahoe debug trial'. This version is better-factored into patches and less dependent on Twisted Trial implementation details. It should also be correctly recorded relative to trunk.

[david-sarah@…]

In [4922/ticket1306]:

src/allmydata/scripts/debug.py: add 'tahoe debug trial' command. refs #1296

[david-sarah@…]

In [4923/ticket1306]:

Tests for 'tahoe debug trial'. refs #1296

[david-sarah@…]

In [4932/ticket1306]:

Since 'tahoe debug trial' requires mock, add it as an unconditional dependency for now. refs #1296

[david-sarah@…]

In [4933/ticket1306]:

setup.py: fix a bug in passing --reporter= argument to trial. refs #1296

[davidsarah]

Needs rebasing for trunk.

[warner]

A preliminary review pass looks good. I definitely prefer the sys.argv=['trial'..] and twisted_trial.run() approach: it feels like that should be reliable. I think I'd rather get the extra arguments from parseArgs() instead of sys.argv[3:] though, as the latter feels fragile. Something like:

   def parseArgs(self, *args):
       self.trial_args = args
...
def trial(config):
    sys.argv = ['trial'] + list(config.trial_args)
    # This does not return
    twisted_trial.run(); sys.exit(0)

(and the sys.exit(0) is for paranoia, to avoid surprises if we're wrong. it's on the same line as twisted_trial.run() keep the code-coverage numbers good. It might be appropriate to raise an exception or do an assert or something there instead.)

I think trial runs just fine without the 'allmydata' or 'allmydata.test' hint: when run without arguments, it scans recursively below the current directory for test files. If an argument *is* necessary, I'd lean towards using 'allmydata' rather than the more-specific 'allmydata.test', as we may want to put tests elsewhere at some point (my #466 signed-introducer work creates allmydata.util.ecdsa.test, for example).

I'd make the help synopsis say "run Tahoe test suite (using Trial)", since users probably care slightly more about what the command does than which particular test framework is involved, and unless you already known Trial, you won't be able to figure out what the command does without a hint.

I'm a little wary of introducing bin/tahoe.pyscript in places we don't need it, especially in the Makefile. I'd be happier if there were some clean way to make TAHOE=bin/tahoe on non-windows. But I know I'm in the minority when it comes to the Makefile, so I'm only a -0 on that.

I'll have to do some comparison testing to see whether I'm happy with changing make quicktest to use the new scheme. It feels like mapping that to 'tahoe debug trial' ought to be slightly faster (one fewer subprocess?).

The other Makefile changes (turning $(PYTHON) bin/tahoe stop into $(TAHOE) stop are probably good ones, but maybe they ought to go into a separate patch, since they don't touch tahoe debug trial directly. (not a separate ticket, just a separate patch, so future code archaeologists have an easier time following the history)

[davidsarah]

Thanks for the review.

Replying to warner:

A preliminary review pass looks good. I definitely prefer the sys.argv=['trial'..] and twisted_trial.run() approach: it feels like that should be reliable. I think I'd rather get the extra arguments from parseArgs() instead of sys.argv[3:] though, as the latter feels fragile. Something like:

   def parseArgs(self, *args):
       self.trial_args = args
...
def trial(config):
    sys.argv = ['trial'] + list(config.trial_args)
    # This does not return
    twisted_trial.run(); sys.exit(0)

That wouldn't include option arguments (and there is no documented way to get the list of option arguments from an Options object). Would sys.argv[len(['tahoe', 'debug', 'trial']):] be more self-documenting?

(and the sys.exit(0) is for paranoia, to avoid surprises if we're wrong.

src/allmydata/runner.py run() would exit anyway if trial(config) were to return.

I think trial runs just fine without the 'allmydata' or 'allmydata.test' hint: when run without arguments, it scans recursively below the current directory for test files.

That would incorrectly catch the src/buildtest module, which is intended only to be run from misc/build_helpers/test-with-fake-dists.py.

If an argument *is* necessary, I'd lean towards using 'allmydata' rather than the more-specific 'allmydata.test', as we may want to put tests elsewhere at some point (my #466 signed-introducer work creates allmydata.util.ecdsa.test, for example).

Hmm, the existing tests of allmydata.util are in allmydata.test.test_util. We can always change this when/if we decide to put tests outside allmydata.test.

I'd make the help synopsis say "run Tahoe test suite (using Trial)", since users probably care slightly more about what the command does than which particular test framework is involved, and unless you already known Trial, you won't be able to figure out what the command does without a hint.

It's not specific to running the Tahoe test suite, that's just the default. Remember that end-users are told to use python setup.py test (or can use python setup.py trial to avoid a build), which then invokes bin/tahoe debug trial. Invoking it directly is a debugging operation, so the description needs to be oriented to debugging (for which it does matter that you're using Twisted Trial, and that the reason for using this rather than the trial script is that the imports are different).

I'll change it to "Run tests using Twisted Trial with correct imports."

I'm a little wary of introducing bin/tahoe.pyscript in places we don't need it, especially in the Makefile.

OK, I'll change this. (Note that bin/tahoe and bin/tahoe.pyscript are identical files on all platforms now; see the last change in [4924/ticket1306].)

I'll have to do some comparison testing to see whether I'm happy with changing make quicktest to use the new scheme. It feels like mapping that to 'tahoe debug trial' ought to be slightly faster (one fewer subprocess?).

There isn't one fewer subprocess, since bin/tahoe creates a subprocess (but that's an implementation detail that might change; for example bin/tahoe could use os.execve on Unix, or it could munge sys.path directly rather than using PYTHONPATH).

The main point of this ticket is that we should only have one way to set up the imports, that is used both for running bin/tahoe normally and for running tests. The other code that was duplicating this set-up (misc/build_helpers/run-with-pythonpath.py and setup.py RunWithPythonPath), can now be deleted.

The other Makefile changes (turning $(PYTHON) bin/tahoe stop into $(TAHOE) stop are probably good ones, but maybe they ought to go into a separate patch, since they don't touch tahoe debug trial directly. (not a separate ticket, just a separate patch, so future code archaeologists have an easier time following the history)

They were in a separate changeset ([4927/ticket1306]), and will be on trunk.

[davidsarah]

Replying to davidsarah:

Replying to warner:

The other Makefile changes (turning $(PYTHON) bin/tahoe stop into $(TAHOE) stop are probably good ones, but maybe they ought to go into a separate patch, since they don't touch tahoe debug trial directly. (not a separate ticket, just a separate patch, so future code archaeologists have an easier time following the history)

They were in a separate changeset ([4927/ticket1306]), and will be on trunk.

Sorry, I misread your comment. Yes, I'll separate out those changes.

[davidsarah]

Replying to warner:

A preliminary review pass looks good. I definitely prefer the sys.argv=['trial'..] and twisted_trial.run() approach: it feels like that should be reliable. I think I'd rather get the extra arguments from parseArgs() instead of sys.argv[3:] though, as the latter feels fragile. Something like:

   def parseArgs(self, *args):
       self.trial_args = args
...
def trial(config):
    sys.argv = ['trial'] + list(config.trial_args)
    # This does not return
    twisted_trial.run(); sys.exit(0)

Your feeling was right; sys.argv[3:] is fragile. In particular it breaks when there are options to tahoe, for example:

bin/tahoe --quiet debug trial allmydata.test.test_base62

will pass "trial allmydata.test.test_base62", to Trial, causing it to attempt to run a non-existent testsuite called "trial", in addition to test_base62. I'll find a more robust solution.

[warner]

Take a look at the internals of usage.Options: I think there's a method to override that will do what you want. In particular, if the 'tahoe debug trial' command doesn't take any options of it's own, I think parseArgs would just give you everything.

[warner]

The following appears to work great, at least for things like "tahoe debug trial", "tahoe debug trial allmydata.test.test_foo", and "tahoe debug trial --random-option random-argument". It depends upon the fact that TrialOptions doesn't actually take any options or arguments: everything gets passed through to trial.run():

class TrialOptions(twisted_trial.Options):
    def getSynopsis(self):
        return "Usage: tahoe debug trial [options] [[file|package|module|TestCase|testmethod]...]"

    def parseOptions(self, options):
        self.options = options

    def getUsage(self, width=None):
        t = twisted_trial.Options.getUsage(self, width)
        t += """
The 'tahoe debug trial' command uses the correct imports for this instance of
Tahoe-LAFS. The default test suite is 'allmydata.test'.
"""
        return t


def trial(config):
    sys.argv = ['trial'] + list(config.options)
    if not config.options:
        sys.argv += ['allmydata.test']

    # This does not return.
    twisted_trial.run()

[davidsarah]

Removing setuptools_trial would have a side-effect described in this comment​:

# Nevow requires Twisted to setup, but doesn't declare that requirement in a
# way that enables setuptools to satisfy that requirement before Nevow's
# setup.py tried to "import twisted". Fortunately we require setuptools_trial
# to setup and setuptools_trial requires Twisted to install, so hopefully
# everything will work out until the Nevow issue is fixed:
# http://divmod.org/trac/ticket/2629 setuptools_trial is needed if you want
# "./setup.py trial" or "./setup.py test" to execute the tests (and in order
# to make sure Twisted is installed early enough -- see the paragraph above).
# http://pypi.python.org/pypi/setuptools_trial
setup_requires.extend(['setuptools_trial >= 0.5'])

(​google cache of the related bug ​http://divmod.org/trac/ticket/2527 while divmod.org is down)

src/allmydata/test/test_nevow.py says that Nevow v0.9.33 fixed that bug. For references, all of the working buildbots have Nevow v0.10.0 except for 'Arthur lenny', which has v0.9.31.

I propose to bump the Nevow version requirement from >= 0.6.0 to >= 0.9.33. Any objection? (In the case of Ubuntu for example, 0.10.0 was packaged for Ubuntu Lucid in April 2010.)

[zooko]

Replying to davidsarah:

src/allmydata/test/test_nevow.py says that Nevow v0.9.33 fixed that bug. For references, all of the working buildbots have Nevow v0.10.0 except for 'Arthur lenny', which has v0.9.31.

I propose to bump the Nevow version requirement from >= 0.6.0 to >= 0.9.33. Any objection?

Does this mean we'd be requiring a version of Nevow newer than what Lenny packages?

​http://packages.debian.org/search?keywords=nevow&searchon=names&suite=all&section=all

Hm, yeah, Lenny has 0.9.31-3. Oh, you know what -- removing the setup-time dependency on setuptools_trial doesn't actually eliminate the setup-time dependency on Twisted, does it? We should probably replace the setup_requires.extend(['setuptools_trial >= 0.5']) with setup_requires.append('Twisted >= 2.4.0'). Maybe that will mean we don't need the newer version of Nevow.

I hope the Arthur lenny buildbot will tell us.

[davidsarah]

Replying to zooko:

Replying to davidsarah:

src/allmydata/test/test_nevow.py says that Nevow v0.9.33 fixed that bug. For references, all of the working buildbots have Nevow v0.10.0 except for 'Arthur lenny', which has v0.9.31.

I propose to bump the Nevow version requirement from >= 0.6.0 to >= 0.9.33. Any objection?

Does this mean we'd be requiring a version of Nevow newer than what Lenny packages?

Yes.

We should probably replace the setup_requires.extend(['setuptools_trial >= 0.5']) with setup_requires.append('Twisted >= 2.4.0'). Maybe that will mean we don't need the newer version of Nevow.

That sounds as though it might work, but I'll try just removing the setuptools_trial dependency first, and see if Arthur lenny breaks.

[zooko]

I'm curious to see what happens with the Arthur lenny buildslave. But, even if it works then it is working by accident, if we don't explicitly specify that Twisted is a setup-time requirement of Tahoe-LAFS (for trial).

[davidsarah]

Code, documentation and test changes, rebased for trunk. Does not include removal of setuptools_trial or changes to 'setup.py trial'

[warner]

my review:

• please add trailing newlines to docs/frontends/CLI.rst andsrc/allmydata/test/trialtest.py
• SystemTest.test_debug_trial fails on my system (with twisted-10.2). I suspect that trial's output has changed in 10.2, you may need to be less strict in matching the output:

56:warner@Cookies% trial --version
Twisted version: 10.2.0
57:warner@Cookies% ./bin/tahoe debug trial allmydata.test.trialtest
allmydata.test.trialtest
  Failure
    test_deferred_error ...                                             [ERROR]
    test_error ...                                                      [ERROR]
    test_fail ...                                                        [FAIL]
  Success
    test_skip ...                                                     [SKIPPED]
    test_succeed ...                                                       [OK]
    test_todo ...                                                        [TODO]

===============================================================================
[SKIPPED]
skip

allmydata.test.trialtest.Success.test_skip
===============================================================================
[TODO]
Reason: 'never mind'
Traceback (most recent call last):
  File "/Users/warner2/stuff/tahoe/trunk/src/allmydata/test/trialtest.py", line 18, in test_todo
    self.fail('umm')
twisted.trial.unittest.FailTest: umm

allmydata.test.trialtest.Success.test_todo
===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/Users/warner2/stuff/tahoe/trunk/src/allmydata/test/trialtest.py", line 24, in test_fail
    self.fail('fail')
twisted.trial.unittest.FailTest: fail

allmydata.test.trialtest.Failure.test_fail
===============================================================================
[ERROR]
Traceback (most recent call last):
Failure: exceptions.AssertionError: screech

allmydata.test.trialtest.Failure.test_deferred_error
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/Users/warner2/stuff/tahoe/trunk/src/allmydata/test/trialtest.py", line 27, in test_error
    raise AssertionError('clang')
exceptions.AssertionError: clang

allmydata.test.trialtest.Failure.test_error
-------------------------------------------------------------------------------
Ran 6 tests in 0.034s

FAILED (skips=1, expectedFailures=1, failures=1, errors=2, successes=1)
58:warner@Cookies% 

Apart from that, it looks awesome. If you can confirm a trial-10.2 fix (you might want to check older versions of Twisted too, back to whatever minimum we support), go ahead and land it.

[david-sarah@…]

In 7a887871b06af4a6:

src/allmydata/scripts/debug.py: add 'tahoe debug trial' command (rebased for trunk). refs #1296

[david-sarah@…]

In 420aadd95efc176e:

Make 'mock' a run-time rather than setup-time dependency. This is necessary in order for 'tahoe debug trial' to work. refs #1296

[david-sarah@…]

In bbc1f56981e7aebf:

Documentation for 'tahoe debug trial' (rebased for trunk). refs #1296

[david-sarah@…]

In 0d6df9c9fc97f756:

(The changeset message doesn't reference this ticket)

[david-sarah@…]

In 1819c25c888ac3e6:

Add src/allmydata/test/trialtest.py needed by tests for 'tahoe debug trial'. refs #1296

[davidsarah]

Replying to zooko:

I'm curious to see what happens with the Arthur lenny buildslave. But, even if it works then it is working by accident, if we don't explicitly specify that Twisted is a setup-time requirement of Tahoe-LAFS (for trial).

Given that Arthur lenny ​has Twisted 8.1.0 installed, it would have worked by accident.

[david-sarah@…]

In 8f0af33ba6cf4f4a:

src/allmydata/test/test_cli.py: add test for 'tahoe debug trial' options help. refs #1296

[david-sarah@…]

In 7e413d4fa45932a8:

Change 'setup.py trial' and 'setup.py test' to use 'bin/tahoe debug trial'. refs #1296

[david-sarah@…]

In 5a7c99d29d85f32d:

(The changeset message doesn't reference this ticket)

[zooko]

I've reviewed 5a7c99d29d85f32d/trunk and it looks good to me.

[david-sarah@…]

In 74b1eec1d661a508:

trivial: add comment in scripts/debug.py about trial option parsing. refs #1296

[zooko]

Re: 7e413d4fa45932a8/trunk, this comment doesn't mention the other reason that we setup-require Twisted which is that we want to ensure that trial is available at test time. Oh, maybe that is best expressed tests_require instead. (But tests_require will probably not be auto-satisfied when you run python setup.py trial the way they would be if you ran python setup.py test.) In any case we should probably specify a build-time or test-time requirement on Twisted.

Other than that, +1 on 7e413d4fa45932a8/trunk!

[zooko]

+1 8f0af33ba6cf4f4a/trunk.

[davidsarah]

Replying to zooko:

Re: 7e413d4fa45932a8/trunk, this comment doesn't mention the other reason that we setup-require Twisted which is that we want to ensure that trial is available at test time.

Actually trial will be available when we run tests because Twisted is a run-time dependency, and bin/tahoe debug trial is a run-time thing from setuptools' point of view (because it's in a separate process that is started by a support script).

From setuptools' point of view, we now don't do anything at test time, other than invoke the Trial command in setup.py.

[david-sarah@…]

In a9fc4668c0e0032c:

docs/frontends/CLI.rst, src/allmydata/test/trialtest.py: add trailing newlines. refs #1296

[david-sarah@…]

In 9ea323db4ccbc778:

Makefile: consistently use 'tahoe debug trial' to run tests. refs #1296

[david-sarah@…]

In d4969259c68bbe77:

(The changeset message doesn't reference this ticket)

[zooko]

So what's left on this ticket? docs-needed? Shall we close the ticket?

[davidsarah]

news-needed, and that's it. Docs were in bbc1f56981e7aebf (and bin/tahoe debug trial --help prints all the options).

[warner]

I'll do a scan of version-control history to generate NEWS for 1.8.2, rather than rely upon news-needed tags. Closing this one out.

[zooko]

The Ubuntu packagers of Tahoe-LAFS didn't notice this change from test-time dep to run-time dep and so Tahoe-LAFS in Natty doesn't declare a dep on python-mock and therefore won't run unless you manually install mock:

​https://bugs.launchpad.net/tahoe-lafs/+bug/782379

[davidsarah]

Replying to zooko:

The Ubuntu packagers of Tahoe-LAFS didn't notice this change from test-time dep ...

​The NEWS for 1.8.2 did note this change. Is there any way we should be making such changes more obvious to packagers?

[zooko]

What would be awesome is if packagers had an automated test bot which would alert them automatically if they made a mistake like this. The NixOS project is the only packaging project I know of that has this:

​http://hydra.nixos.org/job/nixpkgs/trunk/tahoelafs

But aside from encouraging packagers to get automated continuous integration, I don't think there's much more we can do, so let's leave this ticket closed.