[tahoe-lafs-trac-stream] [tahoe-lafs] #1296: 'tahoe debug trial' command

[tahoe-lafs]

#1296: 'tahoe debug trial' command
-----------------------------------+----------------------------------------
     Reporter:  davidsarah         |       Owner:  davidsarah                       
         Type:  defect             |      Status:  assigned                         
     Priority:  major              |   Milestone:  1.8.2                            
    Component:  code-frontend-cli  |     Version:  1.8.1                            
   Resolution:                     |    Keywords:  bbfreeze setuptools review-needed
Launchpad Bug:                     |  
-----------------------------------+----------------------------------------

Comment (by davidsarah):

 Thanks for the review.

 Replying to [comment:9 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.

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

-- 
Ticket URL: <http://tahoe-lafs.org/trac/tahoe-lafs/ticket/1296#comment:10>
tahoe-lafs <http://tahoe-lafs.org>
secure decentralized storage