#1296 closed defect (fixed)

'tahoe debug trial' command

Reported by: davidsarah Owned by: davidsarah
Priority: major Milestone: 1.8.2
Component: code-frontend-cli Version: 1.8.1
Keywords: bbfreeze setuptools Cc:
Launchpad Bug:

Description

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.

Attachments (5)

tahoe-debug-trial.darcs.patch (13.0 KB) - added by davidsarah at 2011-01-07T10:10:51Z.
src/allmydata/scripts/debug.py: add 'tahoe debug trial' command. refs #1296
test-tahoe-debug-trial.darcs.patch (25.1 KB) - added by davidsarah at 2011-01-08T12:19:45Z.
Tests for 'tahoe debug trial'. refs #1296 (includes some dependency patches)
setup-use-tahoe-debug-trial.darcs.patch (26.7 KB) - added by davidsarah at 2011-01-08T12:50:28Z.
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
combined-debug-trial.darcs.patch (36.3 KB) - added by davidsarah at 2011-01-10T07:49:52Z.
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.
code-docs-test-1296.darcs.patch (24.5 KB) - added by davidsarah at 2011-01-18T22:45:24Z.
Code, documentation and test changes, rebased for trunk. Does not include removal of setuptools_trial or changes to 'setup.py trial'

Download all attachments as: .zip

Change History (48)

Changed at 2011-01-07T10:10:51Z by davidsarah

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

comment:1 Changed at 2011-01-07T10:29:04Z by davidsarah

  • Keywords test-needed added; test removed
  • Owner set to davidsarah
  • Status changed from new to assigned

Changed at 2011-01-08T12:19:45Z by davidsarah

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

Changed at 2011-01-08T12:50:28Z by 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

comment:2 Changed at 2011-01-08T12:52:19Z by davidsarah

  • Keywords review-needed added; test-needed removed
  • Owner davidsarah deleted
  • Status changed from assigned to new

comment:3 Changed at 2011-01-10T07:14:51Z by davidsarah

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

Changed at 2011-01-10T07:49:52Z by 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.

comment:4 Changed at 2011-01-14T06:41:08Z by david-sarah@…

In [4922/ticket1306]:

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

comment:5 Changed at 2011-01-14T06:41:09Z by david-sarah@…

In [4923/ticket1306]:

Tests for 'tahoe debug trial'. refs #1296

comment:6 Changed at 2011-01-14T07:52:44Z by david-sarah@…

In [4932/ticket1306]:

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

comment:7 Changed at 2011-01-14T08:18:49Z by david-sarah@…

In [4933/ticket1306]:

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

comment:8 Changed at 2011-01-17T07:27:17Z by davidsarah

  • Owner set to davidsarah
  • Status changed from new to assigned

Needs rebasing for trunk.

comment:9 follow-up: Changed at 2011-01-17T09:54:14Z by 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)

comment:10 in reply to: ↑ 9 ; follow-ups: Changed at 2011-01-17T20:41:40Z by 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.

comment:11 in reply to: ↑ 10 Changed at 2011-01-17T22:13:07Z by 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.

comment:12 in reply to: ↑ 10 Changed at 2011-01-18T04:32:00Z by 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.

comment:13 Changed at 2011-01-18T05:56:50Z by 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.

comment:14 Changed at 2011-01-18T06:28:34Z by 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()

comment:15 follow-up: Changed at 2011-01-18T10:38:13Z by 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.)

Last edited at 2011-01-18T10:40:42Z by davidsarah (previous) (diff)

comment:16 in reply to: ↑ 15 ; follow-up: Changed at 2011-01-18T15:04:59Z by 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.

comment:17 in reply to: ↑ 16 Changed at 2011-01-18T20:30:41Z by 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.

comment:18 follow-up: Changed at 2011-01-18T20:48:40Z by 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).

Changed at 2011-01-18T22:45:24Z by davidsarah

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

comment:19 Changed at 2011-01-19T00:40:50Z by 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.

comment:20 Changed at 2011-01-19T02:15:53Z by david-sarah@…

In 7a887871b06af4a6:

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

comment:21 Changed at 2011-01-19T02:15:54Z by 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

comment:22 Changed at 2011-01-19T02:15:54Z by david-sarah@…

In bbc1f56981e7aebf:

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

comment:23 Changed at 2011-01-19T02:15:55Z by david-sarah@…

In 0d6df9c9fc97f756:

(The changeset message doesn't reference this ticket)

comment:24 Changed at 2011-01-19T02:34:05Z by david-sarah@…

In 1819c25c888ac3e6:

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

comment:25 in reply to: ↑ 18 Changed at 2011-01-19T03:00:52Z by 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.

comment:26 Changed at 2011-01-19T03:14:32Z by david-sarah@…

In 8f0af33ba6cf4f4a:

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

comment:27 Changed at 2011-01-19T03:23:53Z by david-sarah@…

In 7e413d4fa45932a8:

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

comment:28 Changed at 2011-01-19T04:57:12Z by david-sarah@…

In 5a7c99d29d85f32d:

(The changeset message doesn't reference this ticket)

comment:29 Changed at 2011-01-19T06:00:49Z by davidsarah

  • Keywords review-needed removed

comment:30 Changed at 2011-01-19T06:57:50Z by zooko

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

comment:31 Changed at 2011-01-19T07:04:38Z by david-sarah@…

In 74b1eec1d661a508:

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

comment:32 follow-up: Changed at 2011-01-19T07:27:37Z by 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!

comment:34 in reply to: ↑ 32 Changed at 2011-01-19T07:42:09Z by 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.

comment:35 Changed at 2011-01-19T08:57:04Z by david-sarah@…

In a9fc4668c0e0032c:

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

comment:36 Changed at 2011-01-20T00:54:11Z by david-sarah@…

In 9ea323db4ccbc778:

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

comment:37 Changed at 2011-01-20T00:54:11Z by david-sarah@…

In d4969259c68bbe77:

(The changeset message doesn't reference this ticket)

comment:38 Changed at 2011-01-20T09:33:52Z by zooko

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

comment:39 Changed at 2011-01-20T20:20:25Z by davidsarah

  • Keywords news-needed added

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

comment:40 Changed at 2011-01-21T17:08:48Z by warner

  • Keywords news-needed removed
  • Resolution set to fixed
  • Status changed from assigned to closed

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.

comment:41 follow-up: Changed at 2011-05-13T20:12:42Z by 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

comment:42 in reply to: ↑ 41 Changed at 2011-05-14T23:05:48Z by 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?

Version 0, edited at 2011-05-14T23:05:48Z by davidsarah (next)

comment:43 Changed at 2011-06-01T22:47:53Z by 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.

Note: See TracTickets for help on using tickets.