'tahoe debug trial' command #1296

New issue

Closed

opened 2011-01-07 09:33:17 +00:00 by daira · 45 comments

daira commented

2011-01-07 09:33:17 +00:00

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 source: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.

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 source: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.

daira added the

labels 2011-01-07 09:33:17 +00:00

daira added this to the 1.8.2 milestone 2011-01-07 09:33:17 +00:00

daira commented

2011-01-07 10:10:51 +00:00

Author

Attachment tahoe-debug-trial.darcs.patch (13310 bytes) added

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

**Attachment** tahoe-debug-trial.darcs.patch (13310 bytes) added src/allmydata/scripts/debug.py: add 'tahoe debug trial' command. refs #1296

tahoe-debug-trial.darcs.patch

13 KiB

daira self-assigned this 2011-01-07 10:29:04 +00:00

daira commented

2011-01-08 12:19:45 +00:00

Author

Attachment test-tahoe-debug-trial.darcs.patch (25660 bytes) added

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

**Attachment** test-tahoe-debug-trial.darcs.patch (25660 bytes) added Tests for 'tahoe debug trial'. refs #1296 (includes some dependency patches)

test-tahoe-debug-trial.darcs.patch

25 KiB

daira commented

2011-01-08 12:50:28 +00:00

Author

Attachment setup-use-tahoe-debug-trial.darcs.patch (27391 bytes) added

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

**Attachment** setup-use-tahoe-debug-trial.darcs.patch (27391 bytes) added 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

setup-use-tahoe-debug-trial.darcs.patch

27 KiB

daira removed their assignment 2011-01-08 12:52:19 +00:00

daira commented

2011-01-10 07:14:51 +00:00

Author

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

Please look only at [attachment:combined-debug-trial.darcs.patch](/tahoe-lafs/trac/attachments/000078ac-64c8-4e95-2453-4ebba5c202f4) and disregard the others.

daira commented

2011-01-10 07:49:52 +00:00

Author

Attachment combined-debug-trial.darcs.patch (37151 bytes) added

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.

**Attachment** combined-debug-trial.darcs.patch (37151 bytes) added 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.

combined-debug-trial.darcs.patch

36 KiB

daira commented

2011-01-14 06:41:08 +00:00

Author

In [4922/ticket1306]:

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

In [4922/ticket1306]: ``` src/allmydata/scripts/debug.py: add 'tahoe debug trial' command. refs #1296 ```

daira commented

2011-01-14 06:41:09 +00:00

Author

In [4923/ticket1306]:

Tests for 'tahoe debug trial'. refs #1296

In [4923/ticket1306]: ``` Tests for 'tahoe debug trial'. refs #1296 ```

daira commented

2011-01-14 07:52:44 +00:00

Author

In [4932/ticket1306]:

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

In [4932/ticket1306]: ``` Since 'tahoe debug trial' requires mock, add it as an unconditional dependency for now. refs #1296 ```

daira commented

2011-01-14 08:18:49 +00:00

Author

In [4933/ticket1306]:

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

In [4933/ticket1306]: ``` setup.py: fix a bug in passing --reporter= argument to trial. refs #1296 ```

daira commented

2011-01-17 07:27:17 +00:00

Author

Needs rebasing for trunk.

daira self-assigned this 2011-01-17 07:27:17 +00:00

warner commented

2011-01-17 09:54:14 +00:00

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)

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)

daira commented

2011-01-17 20:41:40 +00:00

Author

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['debug', 'trial']len(['tahoe',):] be more self-documenting?

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

source: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 source:src/buildtest module, which is intended only to be run from source: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 (source:misc/build_helpers/run-with-pythonpath.py and source: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.

Thanks for the review. Replying to [warner](/tahoe-lafs/trac/issues/1296#issuecomment-382857): > 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['debug', 'trial']len(['tahoe',):]` be more self-documenting? > (and the `sys.exit(0)` is for paranoia, to avoid surprises if we're wrong. source: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 source:src/buildtest module, which is intended only to be run from source: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 (source:misc/build_helpers/run-with-pythonpath.py and source: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.

daira commented

2011-01-17 22:13:07 +00:00

Author

Replying to [davidsarah]comment:10:

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.

Replying to [davidsarah]comment:10: > Replying to [warner](/tahoe-lafs/trac/issues/1296#issuecomment-382857): > > 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.

daira commented

2011-01-18 04:32:00 +00:00

Author

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.

Replying to [warner](/tahoe-lafs/trac/issues/1296#issuecomment-382857): > 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 commented

2011-01-18 05:56:50 +00:00

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.

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 commented

2011-01-18 06:28:34 +00:00

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()

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() ```

daira commented

2011-01-18 10:38:13 +00:00

Author

Removing setuptools_trial would have a side-effect described in [this comment]source:setup.py@4940#L143:

# 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)

source: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.)

Removing setuptools_trial would have a side-effect described in [this comment]source:setup.py@4940#L143: ``` # 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](https://webcache.googleusercontent.com/search?q=cache:divmod.org/trac/ticket/2527) of the related bug <http://divmod.org/trac/ticket/2527> while divmod.org is down) source: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 commented

2011-01-18 15:04:59 +00:00

Replying to davidsarah:

source: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.

Replying to [davidsarah](/tahoe-lafs/trac/issues/1296#issuecomment-382863): > source: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.

daira commented

2011-01-18 20:30:41 +00:00

Author

Replying to [zooko]comment:16:

Replying to davidsarah:

source: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.

Replying to [zooko]comment:16: > Replying to [davidsarah](/tahoe-lafs/trac/issues/1296#issuecomment-382863): > > > source: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 commented

2011-01-18 20:48:40 +00:00

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).

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`).

daira commented

2011-01-18 22:45:24 +00:00

Author

Attachment code-docs-test-1296.darcs.patch (25100 bytes) added

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

**Attachment** code-docs-test-1296.darcs.patch (25100 bytes) added Code, documentation and test changes, rebased for trunk. Does not include removal of setuptools_trial or changes to 'setup.py trial'

code-docs-test-1296.darcs.patch

24 KiB

warner commented

2011-01-19 00:40:50 +00:00

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.

my review: * please add trailing newlines to `docs/frontends/CLI.rst` and`src/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.

daira commented

2011-01-19 02:15:53 +00:00

Author

In changeset:7a887871b06af4a6:

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

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

daira commented

2011-01-19 02:15:54 +00:00

Author

In changeset:420aadd95efc176e:

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

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

daira commented

2011-01-19 02:15:54 +00:00

Author

In changeset:bbc1f56981e7aebf:

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

In changeset:bbc1f56981e7aebf: ``` Documentation for 'tahoe debug trial' (rebased for trunk). refs #1296 ```

daira commented

2011-01-19 02:15:55 +00:00

Author

In changeset:0d6df9c9fc97f756:

Tests for 'tahoe debug trial' (rebased and fixed to work with Twisted 10.2). refs #1296

In changeset:0d6df9c9fc97f756: ``` Tests for 'tahoe debug trial' (rebased and fixed to work with Twisted 10.2). refs #1296 ```

daira commented

2011-01-19 02:34:05 +00:00

Author

In changeset:1819c25c888ac3e6:

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

In changeset:1819c25c888ac3e6: ``` Add src/allmydata/test/trialtest.py needed by tests for 'tahoe debug trial'. refs #1296 ```

daira commented

2011-01-19 03:00:52 +00:00

Author

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.

Replying to [zooko](/tahoe-lafs/trac/issues/1296#issuecomment-382866): > 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](http://tahoe-lafs.org/buildbot/builders/Arthur%20lenny%20c7%2032bit/builds/656/steps/show-tool-versions/logs/stdio), it would have worked by accident.

daira commented

2011-01-19 03:14:32 +00:00

Author

In changeset:8f0af33ba6cf4f4a:

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

In changeset:8f0af33ba6cf4f4a: ``` src/allmydata/test/test_cli.py: add test for 'tahoe debug trial' options help. refs #1296 ```

daira commented

2011-01-19 03:23:53 +00:00

Author

In changeset:7e413d4fa45932a8:

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

In changeset:7e413d4fa45932a8: ``` Change 'setup.py trial' and 'setup.py test' to use 'bin/tahoe debug trial'. refs #1296 ```

daira commented

2011-01-19 04:57:12 +00:00

Author

In changeset:5a7c99d29d85f32d:

Change misc/build_helpers/test-with-fake-pkg to use 'setup.py trial'. refs #1296

In changeset:5a7c99d29d85f32d: ``` Change misc/build_helpers/test-with-fake-pkg to use 'setup.py trial'. refs #1296 ```

zooko commented

2011-01-19 06:57:50 +00:00

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

daira commented

2011-01-19 07:04:38 +00:00

Author

In changeset:74b1eec1d661a508:

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

In changeset:74b1eec1d661a508: ``` trivial: add comment in scripts/debug.py about trial option parsing. refs #1296 ```

zooko commented

2011-01-19 07:27:37 +00:00

Re: changeset:7e413d4fa45932a8/trunk, [this comment]source:trunk/setup.py@4950#L138 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]source:trunk/setup.py@4950#L164 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 changeset:7e413d4fa45932a8/trunk!

Re: changeset:7e413d4fa45932a8/trunk, [this comment]source:trunk/setup.py@4950#L138 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]source:trunk/setup.py@4950#L164 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 changeset:7e413d4fa45932a8/trunk!

zooko commented

2011-01-19 07:32:52 +00:00

+1 changeset:8f0af33ba6cf4f4a/trunk.

daira commented

2011-01-19 07:42:09 +00:00

Author

Replying to zooko:

Re: changeset:7e413d4fa45932a8/trunk, [this comment]source:trunk/setup.py@4950#L138 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 source:setup.py.

Replying to [zooko](/tahoe-lafs/trac/issues/1296#issuecomment-382879): > Re: changeset:7e413d4fa45932a8/trunk, [this comment]source:trunk/setup.py@4950#L138 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 source:setup.py.

daira commented

2011-01-19 08:57:04 +00:00

Author

In changeset:a9fc4668c0e0032c:

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

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

daira commented

2011-01-20 00:54:11 +00:00

Author

In changeset:9ea323db4ccbc778:

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

In changeset:9ea323db4ccbc778: ``` Makefile: consistently use 'tahoe debug trial' to run tests. refs #1296 ```

daira commented

2011-01-20 00:54:11 +00:00

Author

In changeset:d4969259c68bbe77:

Makefile: consistently use TAHOE macro to run bin/tahoe. Use '$(TAHOE) debug repl' instead of $(RUNPP) -p. refs #1296

In changeset:d4969259c68bbe77: ``` Makefile: consistently use TAHOE macro to run bin/tahoe. Use '$(TAHOE) debug repl' instead of $(RUNPP) -p. refs #1296 ```

zooko commented

2011-01-20 09:33:52 +00:00

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

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

daira commented

2011-01-20 20:20:25 +00:00

Author

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

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

warner commented

2011-01-21 17:08:48 +00:00

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.

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.

warner added the

r/fixed

label 2011-01-21 17:08:48 +00:00

warner closed this issue

2011-01-21 17:08:48 +00:00

zooko commented

2011-05-13 20:12:42 +00:00

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

daira commented

2011-05-14 23:05:48 +00:00

Author

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?

Replying to [zooko](/tahoe-lafs/trac/issues/1296#issuecomment-382890): > The Ubuntu packagers of Tahoe-LAFS didn't notice this change from test-time dep ... [The NEWS for 1.8.2](http://tahoe-lafs.org/trac/tahoe-lafs/changeset/4992) did note this change. Is there any way we should be making such changes more obvious to packagers?

zooko commented

2011-06-01 22:47:53 +00:00

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.

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.