Repository: google/pprof
Pull request #519 (2020-04-08 21:57:45)

Provide more helpful error message and require input for every non-bool option.
Fixes: #441

1. More helpful error messages and usages are added.
2. All non-bool option now requires an input. That means "call_tree" is valid, but "sample_index" is invalid.

Before:
```
(pprof) sample_index
(pprof)
```

```
(pprof) sample_index 0
unrecognized command: "sample_index"
```

```
(pprof) help
    ...
    sample_index     Sample value to report (0-based index or name)
    ...
```

After:
```
(pprof) sample_index
please input a value, e.g. sample_index=<val>
(pprof) focus
please input a value, e.g. focus=<val>
(pprof) call_tree
(pprof) 
```

```
(pprof) sample_index 0
do you mean: "sample_index=0"?
```

codecov-io: # [Codecov](https://codecov.io/gh/google/pprof/pull/519?src=pr&el=h1) Report
> Merging [#519](https://codecov.io/gh/google/pprof/pull/519?src=pr&el=desc) into [master](https://codecov.io/gh/google/pprof/commit/d6d2dc122c202476d1eacc56b587de6a95d4fae5&el=desc) will **decrease** coverage by `0.06%`.
> The diff coverage is `21.42%`.

[![Impacted file tree graph](https://codecov.io/gh/google/pprof/pull/519/graphs/tree.svg?width=650&height=150&src=pr&token=nHgs4Smibv)](https://codecov.io/gh/google/pprof/pull/519?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
- Coverage   67.07%   67.01%   -0.07%     
==========================================
  Files          37       37              
  Lines        7615     7627      +12     
==========================================
+ Hits         5108     5111       +3     
- Misses       2103     2111       +8     
- Partials      404      405       +1     
```


| [Impacted Files](https://codecov.io/gh/google/pprof/pull/519?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/driver/commands.go](https://codecov.io/gh/google/pprof/pull/519/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZHJpdmVyL2NvbW1hbmRzLmdv) | `42.51% <0.00%> (-1.49%)` | :arrow_down: |
| [internal/driver/interactive.go](https://codecov.io/gh/google/pprof/pull/519/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZHJpdmVyL2ludGVyYWN0aXZlLmdv) | `60.15% <60.00%> (-0.01%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/pprof/pull/519?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/pprof/pull/519?src=pr&el=footer). Last update [d6d2dc1...2a191ba](https://codecov.io/gh/google/pprof/pull/519?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

aalexand: Could you include "before" and "after" outputs in the PR description? Basically, enough data for the reviewer to convince themselves that the bug is fixed on all its points.
aalexand: On

```
(pprof) sample_index 0
Unrecognized command: "sample_index"
```

vs.

```
(pprof) sample_index
please input a value, e.g. option = value

(pprof) focus
please input a value, e.g. option = value

(pprof) sample_index 0
do you mean: "sample_index=0"?
```

Casing is inconsistent. User visible messages should use sentence case.
wyk9787: > On
> 
> ```
> (pprof) sample_index 0
> Unrecognized command: "sample_index"
> ```
> 
> vs.
> 
> ```
> (pprof) sample_index
> please input a value, e.g. option = value
> 
> (pprof) focus
> please input a value, e.g. option = value
> 
> (pprof) sample_index 0
> do you mean: "sample_index=0"?
> ```
> 
> Casing is inconsistent. User visible messages should use sentence case.

Actually before was lowercase as well and was consistent throughout the code base, i.e. "unrecognized command ...". Also, my IDE reminds me that error message should not start with uppercase, https://github.com/golang/go/wiki/CodeReviewComments#error-strings
aalexand: > > On
> > ```
> > (pprof) sample_index 0
> > Unrecognized command: "sample_index"
> > ```
> > 
> > 
> > vs.
> > ```
> > (pprof) sample_index
> > please input a value, e.g. option = value
> > 
> > (pprof) focus
> > please input a value, e.g. option = value
> > 
> > (pprof) sample_index 0
> > do you mean: "sample_index=0"?
> > ```
> > 
> > 
> > Casing is inconsistent. User visible messages should use sentence case.
> 
> Actually before was lowercase as well and was consistent throughout the code base, i.e. "unrecognized command ...". Also, my IDE reminds me that error message should not start with uppercase, https://github.com/golang/go/wiki/CodeReviewComments#error-strings

@wyk9787 I took the example from the PR description. It's fine to keep these lowercase, but the casing should be consistent.
wyk9787: > > > On
> > > ```
> > > (pprof) sample_index 0
> > > Unrecognized command: "sample_index"
> > > ```
> > > 
> > > 
> > > vs.
> > > ```
> > > (pprof) sample_index
> > > please input a value, e.g. option = value
> > > 
> > > (pprof) focus
> > > please input a value, e.g. option = value
> > > 
> > > (pprof) sample_index 0
> > > do you mean: "sample_index=0"?
> > > ```
> > > 
> > > 
> > > Casing is inconsistent. User visible messages should use sentence case.
> > 
> > 
> > Actually before was lowercase as well and was consistent throughout the code base, i.e. "unrecognized command ...". Also, my IDE reminds me that error message should not start with uppercase, https://github.com/golang/go/wiki/CodeReviewComments#error-strings
> 
> @wyk9787 I took the example from the PR description. It's fine to keep these lowercase, but the casing should be consistent.

@aalexand Sorry about! Description is updated.
aalexand: Looking at the full output (below), I find the options section a bit too noisy on the left side now. I wonder if it would be sufficient to mention the option type in the parens after the option description or something like that. I don't think having the placeholder in the exact place of the option description text is that valuable - most command-line tools don't have it in their "--help" output and it works fairly well.

Also, there are flags like -timeout, -seconds and other which also accept values, and now their description is not consistent with the options part. This is another reason I would try to make this PR change the help formatting for options a bit less, to make sure the whole -help output is self-coherent and consistent.

Also summoning @rauls5382 for his opinion.

```
$ go run pprof.go
usage:

Produce output in the specified format.

   pprof <format> [options] [binary] <source> ...

Omit the format to get an interactive shell whose commands can be used
to generate various views of a profile

   pprof [options] [binary] <source> ...

Omit the format and provide the "-http" flag to get an interactive web
interface at the specified host:port that can be used to navigate through
various views of a profile.

   pprof -http [host]:[port] [options] [binary] <source> ...

Details:
  Output formats (select at most one):
    -callgrind       Outputs a graph in callgrind format
    -comments        Output all profile comments
    -disasm          Output assembly listings annotated with samples
    -dot             Outputs a graph in DOT format
    -eog             Visualize graph through eog
    -evince          Visualize graph through evince
    -gif             Outputs a graph image in GIF format
    -gv              Visualize graph through gv
    -kcachegrind     Visualize report in KCachegrind
    -list            Output annotated source for functions matching regexp
    -pdf             Outputs a graph in PDF format
    -peek            Output callers/callees of functions matching regexp
    -png             Outputs a graph image in PNG format
    -proto           Outputs the profile in compressed protobuf format
    -ps              Outputs a graph in PS format
    -raw             Outputs a text representation of the raw profile
    -svg             Outputs a graph in SVG format
    -tags            Outputs all tags in the profile
    -text            Outputs top entries in text form
    -top             Outputs top entries in text form
    -topproto        Outputs top entries in compressed protobuf format
    -traces          Outputs all profile samples in text form
    -tree            Outputs a text rendering of call graph
    -web             Visualize graph through web browser
    -weblist         Display annotated source in a web browser

  Options:
    General format is <option>=<value>
    <f> is a float, <n> is an integer, and 
 is a string
    Use <option> or <option>=t to set a boolean flag to true and <option>=f to set it to false

    -call_tree       Create a context-sensitive call tree
    -compact_labels  Show minimal headers
    -divide_by=<f>   <f> is the ratio to divide all samples before visualization
    -drop_negative   Ignore negative differences
    -edgefraction=<f> Hide edges below <f>*total
    -focus=
       Restricts to samples going through a node matching regexp 

    -hide=
        Skips nodes matching regexp 

    -ignore=
      Skips paths going through any nodes matching regexp 

    -mean            Average sample value over first value (count)
    -nodecount=<n>   <n> is the max number of nodes to show
    -nodefraction=<f> Hide nodes below <f>*total
    -noinlines       Ignore inlines.
    -normalize       Scales profile based on the base profile.
    -output=
      
 is the output filename for file-based outputs
    -prune_from=
  Drops any functions below the matched frame 
.
    -relative_percentages Show percentages relative to focused subgraph
    -sample_index=<x> Sample value to report (<x> is 0-based index or name)
    -show=
        Only show nodes matching regexp 

    -show_from=
   Drops functions above the highest matched frame 
.
    -source_path=
 Search path 
 for source files
    -tagfocus=
    Restricts to samples with tags in range or matched by regexp 

    -taghide=
     Skip tags matching this regexp 

    -tagignore=
   Discard samples with tags in range or matched by regexp 

    -tagshow=
     Only consider tags matching this regexp 

    -trim            Honor nodefraction/edgefraction/nodecount defaults
    -trim_path=
   Path 
 to trim from source paths before search
    -unit=
        
 is the measurement unit to display

  Option groups (only set one per group):
    cumulative
      -cum             Sort entries based on cumulative weight
      -flat            Sort entries based on own weight
    granularity
      -addresses       Aggregate at the address level.
      -filefunctions   Aggregate at the function level.
      -files           Aggregate at the file level.
      -functions       Aggregate at the function level.
      -lines           Aggregate at the source code line level.

  Source options:
    -seconds              Duration for time-based profile collection
    -timeout              Timeout in seconds for profile collection
    -buildid              Override build id for main binary
    -add_comment          Free-form annotation to add to the profile
                          Displayed on some reports or with pprof -comments
    -diff_base source     Source of base profile for comparison
    -base source          Source of base profile for profile subtraction
    profile.pb.gz         Profile in compressed protobuf format
    legacy_profile        Profile in legacy pprof format
    http://host/profile   URL for profile handler to retrieve
    -symbolize=           Controls source of symbol information
      none                  Do not attempt symbolization
      local                 Examine only local binaries
      fastlocal             Only get function names from local binaries
      remote                Do not examine local binaries
      force                 Force re-symbolization
    Binary                  Local path or build id of binary for symbolization
    -tls_cert             TLS client certificate file for fetching profile and symbols
    -tls_key              TLS private key file for fetching profile and symbols
    -tls_ca               TLS CA certs file for fetching profile and symbols

  Misc options:
   -http              Provide web interface at host:port.
                      Host is optional and 'localhost' by default.
                      Port is optional and a randomly available port by default.
   -no_browser        Skip opening a browser for the interactive web UI.
   -tools             Search path for object tools

  Legacy convenience options:
   -inuse_space           Same as -sample_index=inuse_space
   -inuse_objects         Same as -sample_index=inuse_objects
   -alloc_space           Same as -sample_index=alloc_space
   -alloc_objects         Same as -sample_index=alloc_objects
   -total_delay           Same as -sample_index=delay
   -contentions           Same as -sample_index=contentions
   -mean_delay            Same as -mean -sample_index=delay

  Environment Variables:
   PPROF_TMPDIR       Location for saved profiles (default $HOME/pprof)
   PPROF_TOOLS        Search path for object-level tools
   PPROF_BINARY_PATH  Search path for local binary files
                      default: $HOME/pprof/binaries
                      searches $name, $path, $buildid/$name, $path/$buildid
   * On Windows, %USERPROFILE% is used instead of $HOME
pprof: no profile source specified
exit status 2
```
wyk9787: > mand-line tools don't have it in their "--help" output and it works fairly well.
> 
> Also, there are flags like -timeout, -seconds and other which also accept values, and now their description is not consist

Sorry, I forgot to change back to 22 spaces after switching back to 16 spaces. I have changed back to 22 spaces now. Do we still need to take out the placeholders?

```
$ go run pprof.go
usage:

Produce output in the specified format.

   pprof <format> [options] [binary] <source> ...

Omit the format to get an interactive shell whose commands can be used
to generate various views of a profile

   pprof [options] [binary] <source> ...

Omit the format and provide the "-http" flag to get an interactive web
interface at the specified host:port that can be used to navigate through
various views of a profile.

   pprof -http [host]:[port] [options] [binary] <source> ...

Details:
  Output formats (select at most one):
    -callgrind             Outputs a graph in callgrind format
    -comments              Output all profile comments
    -disasm                Output assembly listings annotated with samples
    -dot                   Outputs a graph in DOT format
    -eog                   Visualize graph through eog
    -evince                Visualize graph through evince
    -gif                   Outputs a graph image in GIF format
    -gv                    Visualize graph through gv
    -kcachegrind           Visualize report in KCachegrind
    -list                  Output annotated source for functions matching regexp
    -pdf                   Outputs a graph in PDF format
    -peek                  Output callers/callees of functions matching regexp
    -png                   Outputs a graph image in PNG format
    -proto                 Outputs the profile in compressed protobuf format
    -ps                    Outputs a graph in PS format
    -raw                   Outputs a text representation of the raw profile
    -svg                   Outputs a graph in SVG format
    -tags                  Outputs all tags in the profile
    -text                  Outputs top entries in text form
    -top                   Outputs top entries in text form
    -topproto              Outputs top entries in compressed protobuf format
    -traces                Outputs all profile samples in text form
    -tree                  Outputs a text rendering of call graph
    -web                   Visualize graph through web browser
    -weblist               Display annotated source in a web browser

  Options:
    General format is <option>=<value>
    <f> is a float, <n> is an integer, and 
 is a string
    Use <option> or <option>=t to set a boolean flag to true and <option>=f to set it to false

    -call_tree             Create a context-sensitive call tree
    -compact_labels        Show minimal headers
    -divide_by=<f>         <f> is the ratio to divide all samples before visualization
    -drop_negative         Ignore negative differences
    -edgefraction=<f>      Hide edges below <f>*total
    -focus=
             Restricts to samples going through a node matching regexp 

    -hide=
              Skips nodes matching regexp 

    -ignore=
            Skips paths going through any nodes matching regexp 

    -mean                  Average sample value over first value (count)
    -nodecount=<n>         <n> is the max number of nodes to show
    -nodefraction=<f>      Hide nodes below <f>*total
    -noinlines             Ignore inlines.
    -normalize             Scales profile based on the base profile.
    -output=
            
 is the output filename for file-based outputs
    -prune_from=
        Drops any functions below the matched frame 
.
    -relative_percentages  Show percentages relative to focused subgraph
    -sample_index=<x>      Sample value to report (<x> is 0-based index or name)
    -show=
              Only show nodes matching regexp 

    -show_from=
         Drops functions above the highest matched frame 
.
    -source_path=
       Search path 
 for source files
    -tagfocus=
          Restricts to samples with tags in range or matched by regexp 

    -taghide=
           Skip tags matching this regexp 

    -tagignore=
         Discard samples with tags in range or matched by regexp 

    -tagshow=
           Only consider tags matching this regexp 

    -trim                  Honor nodefraction/edgefraction/nodecount defaults
    -trim_path=
         Path 
 to trim from source paths before search
    -unit=
              
 is the measurement unit to display

  Option groups (only set one per group):
    cumulative             
      -cum                   Sort entries based on cumulative weight
      -flat                  Sort entries based on own weight
    granularity            
      -addresses             Aggregate at the address level.
      -filefunctions         Aggregate at the function level.
      -files                 Aggregate at the file level.
      -functions             Aggregate at the function level.
      -lines                 Aggregate at the source code line level.

  Source options:
    -seconds              Duration for time-based profile collection
    -timeout              Timeout in seconds for profile collection
    -buildid              Override build id for main binary
    -add_comment          Free-form annotation to add to the profile
                          Displayed on some reports or with pprof -comments
    -diff_base source     Source of base profile for comparison
    -base source          Source of base profile for profile subtraction
    profile.pb.gz         Profile in compressed protobuf format
    legacy_profile        Profile in legacy pprof format
    http://host/profile   URL for profile handler to retrieve
    -symbolize=           Controls source of symbol information
      none                  Do not attempt symbolization
      local                 Examine only local binaries
      fastlocal             Only get function names from local binaries
      remote                Do not examine local binaries
      force                 Force re-symbolization
    Binary                  Local path or build id of binary for symbolization
    -tls_cert             TLS client certificate file for fetching profile and symbols
    -tls_key              TLS private key file for fetching profile and symbols
    -tls_ca               TLS CA certs file for fetching profile and symbols

  Misc options:
   -http              Provide web interface at host:port.
                      Host is optional and 'localhost' by default.
                      Port is optional and a randomly available port by default.
   -no_browser        Skip opening a browser for the interactive web UI.
   -tools             Search path for object tools

  Legacy convenience options:
   -inuse_space           Same as -sample_index=inuse_space
   -inuse_objects         Same as -sample_index=inuse_objects
   -alloc_space           Same as -sample_index=alloc_space
   -alloc_objects         Same as -sample_index=alloc_objects
   -total_delay           Same as -sample_index=delay
   -contentions           Same as -sample_index=contentions
   -mean_delay            Same as -mean -sample_index=delay

  Environment Variables:
   PPROF_TMPDIR       Location for saved profiles (default $HOME/pprof)
   PPROF_TOOLS        Search path for object-level tools
   PPROF_BINARY_PATH  Search path for local binary files
                      default: $HOME/pprof/binaries
                      searches $name, $path, $buildid/$name, $path/$buildid
   * On Windows, %USERPROFILE% is used instead of $HOME
pprof: no profile source specified
exit status 2
```
aalexand: > Sorry, I forgot to change back to 22 spaces after switching back to 16 spaces. I have changed back to 22 spaces now. Do we still need to take out the placeholders?

Yes, I think so: my concern was not related to the wrapping / unaligned text.
wyk9787: > > Sorry, I forgot to change back to 22 spaces after switching back to 16 spaces. I have changed back to 22 spaces now. Do we still need to take out the placeholders?
> 
> Yes, I think so: my concern was not related to the wrapping / unaligned text.

Got it. Just to clarify, for this PR, are you expecting something like:
```
-nodecount         Max number of nodes to show (n)
-show                   Only show nodes matching regexp (s)
```
for all options and commands that require a parameter? Or, I should simply discard those changes for the help in this PR?

wyk9787: Reverted all modifications to the help text and updated description.
aalexand: > Or, I should simply discard those changes for the help in this PR?

I would probably leave out that part of changing the help message. (And revert the header of the options section as needed)
wyk9787: > > Or, I should simply discard those changes for the help in this PR?
> 
> I would probably leave out that part of changing the help message. (And revert the header of the options section as needed)

Yes, already reverted all changes made to the help message. There is no modification to the help message in this PR and the description is updated as well.
aalexand: @wyk9787 The travis build is failing?
wyk9787: > @wyk9787 The travis build is failing?

 It seems like the Travis times out before the test is even run: https://travis-ci.org/github/google/pprof/jobs/675881270.

Is it possible to increase the time limit for travis? Currently it's set to 50 minutes.
nolanmar511: > > @wyk9787 The travis build is failing?
> 
> It seems like the Travis times out before the test is even run: https://travis-ci.org/github/google/pprof/jobs/675881270.
> 
> Is it possible to increase the time limit for travis? Currently it's set to 50 minutes.

For now, I restarted the errored tests. Can other folks restart these tests, too?
wyk9787: > > > @wyk9787 The travis build is failing?
> > 
> > 
> > It seems like the Travis times out before the test is even run: https://travis-ci.org/github/google/pprof/jobs/675881270.
> > Is it possible to increase the time limit for travis? Currently it's set to 50 minutes.
> 
> For now, I restarted the errored tests. Can other folks restart these tests, too?

I don't think I am able to. 
wyk9787: @aalexand The Travis build just passed.

Patch:
diff --git a/internal/driver/interactive.go b/internal/driver/interactive.go
index 3a458b0..e2d60c6 100644
--- a/internal/driver/interactive.go
+++ b/internal/driver/interactive.go
@@ -70,6 +70,11 @@ func interactive(p *profile.Profile, o *plugin.Options) error {
 					value = strings.TrimSpace(value)
 				}
 				if v := pprofVariables[name]; v != nil {
+					// All non-bool options require inputs
+					if v.kind != boolKind && value == "" {
+						o.UI.PrintErr(fmt.Errorf("please specify a value, e.g. %s=<val>", name))
+						continue
+					}
 					if name == "sample_index" {
 						// Error check sample_index=xxx to ensure xxx is a valid sample type.
 						index, err := p.SampleIndexByName(value)
@@ -267,6 +272,9 @@ func parseCommandLine(input []string) ([]string, variables, error) {
 		}
 	}
 	if c == nil {
+		if v := pprofVariables[name]; v != nil {
+			return nil, nil, fmt.Errorf("did you mean: %s=%s", name, args[0])
+		}
 		return nil, nil, fmt.Errorf("unrecognized command: %q", name)
 	}
 


Test patch:
diff --git a/internal/driver/interactive_test.go b/internal/driver/interactive_test.go
index d8e4ef4..903d18e 100644
--- a/internal/driver/interactive_test.go
+++ b/internal/driver/interactive_test.go
@@ -40,62 +40,53 @@ func TestShell(t *testing.T) {
 	savedVariables := pprofVariables
 	defer func() { pprofVariables = savedVariables }()
 
-	// Random interleave of independent scripts
-	pprofVariables = testVariables(savedVariables)
+	shortcuts1, scScript1 := makeShortcuts(interleave(script, 2), 1)
+	shortcuts2, scScript2 := makeShortcuts(interleave(script, 1), 2)
 
-	// pass in HTTPTransport when setting defaults, because otherwise default
-	// transport will try to add flags to the default flag set.
-	o := setDefaults(&plugin.Options{HTTPTransport: transport.New(nil)})
-	o.UI = newUI(t, interleave(script, 0))
-	if err := interactive(p, o); err != nil {
-		t.Error("first attempt:", err)
-	}
-	// Random interleave of independent scripts
-	pprofVariables = testVariables(savedVariables)
-	o.UI = newUI(t, interleave(script, 1))
-	if err := interactive(p, o); err != nil {
-		t.Error("second attempt:", err)
+	var testcases = []struct {
+		name              string
+		input             []string
+		shortcuts         shortcuts
+		allowRx           string
+		numAllowRxMatches int
+		propagateError    bool
+	}{
+		{"Random interleave of independent scripts 1", interleave(script, 0), pprofShortcuts, "", 0, false},
+		{"Random interleave of independent scripts 2", interleave(script, 1), pprofShortcuts, "", 0, false},
+		{"Random interleave of independent scripts with shortcuts 1", scScript1, shortcuts1, "", 0, false},
+		{"Random interleave of independent scripts with shortcuts 2", scScript2, shortcuts2, "", 0, false},
+		{"Group with invalid value", []string{"cumulative=this"}, pprofShortcuts, `unrecognized value for cumulative: "this". Use one of cum, flat`, 1, false},
+		{"No special value provided for the option", []string{"sample_index"}, pprofShortcuts, `please specify a value, e.g. sample_index=<val>`, 1, false},
+		{"No string value provided for the option", []string{"focus="}, pprofShortcuts, `please specify a value, e.g. focus=<val>`, 1, false},
+		{"No float value provided for the option", []string{"divide_by"}, pprofShortcuts, `please specify a value, e.g. divide_by=<val>`, 1, false},
+		{"Helpful input format reminder", []string{"sample_index 0"}, pprofShortcuts, `did you mean: sample_index=0`, 1, false},
+		{"Verify propagation of IO errors", []string{"**error**"}, pprofShortcuts, "", 0, true},
 	}
 
-	// Random interleave of independent scripts with shortcuts
-	pprofVariables = testVariables(savedVariables)
-	var scScript []string
-	pprofShortcuts, scScript = makeShortcuts(interleave(script, 2), 1)
-	o.UI = newUI(t, scScript)
-	if err := interactive(p, o); err != nil {
-		t.Error("first shortcut attempt:", err)
-	}
+	o := setDefaults(&plugin.Options{HTTPTransport: transport.New(nil)})
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			pprofVariables = testVariables(savedVariables)
+			pprofShortcuts = tc.shortcuts
+			ui := &proftest.TestUI{
+				T:       t,
+				Input:   tc.input,
+				AllowRx: tc.allowRx,
+			}
+			o.UI = ui
 
-	// Random interleave of independent scripts with shortcuts
-	pprofVariables = testVariables(savedVariables)
-	pprofShortcuts, scScript = makeShortcuts(interleave(script, 1), 2)
-	o.UI = newUI(t, scScript)
-	if err := interactive(p, o); err != nil {
-		t.Error("second shortcut attempt:", err)
-	}
+			err := interactive(p, o)
+			if (tc.propagateError && err == nil) || (!tc.propagateError && err != nil) {
+				t.Errorf("%s: %v", tc.name, err)
+			}
 
-	// Group with invalid value
-	pprofVariables = testVariables(savedVariables)
-	ui := &proftest.TestUI{
-		T:       t,
-		Input:   []string{"cumulative=this"},
-		AllowRx: `unrecognized value for cumulative: "this". Use one of cum, flat`,
-	}
-	o.UI = ui
-	if err := interactive(p, o); err != nil {
-		t.Error("invalid group value:", err)
-	}
-	// Confirm error message written out once.
-	if ui.NumAllowRxMatches != 1 {
-		t.Errorf("want error message to be printed 1 time, got %v", ui.NumAllowRxMatches)
-	}
-	// Verify propagation of IO errors
-	pprofVariables = testVariables(savedVariables)
-	o.UI = newUI(t, []string{"**error**"})
-	if err := interactive(p, o); err == nil {
-		t.Error("expected IO error, got nil")
+			// Confirm error message written out once.
+			if tc.numAllowRxMatches != ui.NumAllowRxMatches {
+				t.Errorf("want error message to be printed %d time(s), got %d",
+					tc.numAllowRxMatches, ui.NumAllowRxMatches)
+			}
+		})
 	}
-
 }
 
 var testCommands = commands{
@@ -132,7 +123,7 @@ var script = []string{
 	"f=-1;f=-2.5;check f=-2.5;f=0.0001;check f=0.0001",
 	"check ff=0;ff=-1.01;check ff=-1.01;ff=100;check ff=100",
 	"s=one;s=two;check s=two",
-	"ss=tree;check ss=tree;ss=;check ss;ss=forest;check ss=forest",
+	"ss=tree;check ss=tree;ss=forest;check ss=forest",
 	"ta=true;check ta=true;check tb=false;check tc=false;tb=1;check tb=true;check ta=false;check tc=false;tc=yes;check tb=false;check ta=false;check tc=true",
 }
 
@@ -162,13 +153,6 @@ func makeShortcuts(input []string, seed int) (shortcuts, []string) {
 	return s, output
 }
 
-func newUI(t *testing.T, input []string) plugin.UI {
-	return &proftest.TestUI{
-		T:     t,
-		Input: input,
-	}
-}
-
 func checkValue(p *profile.Profile, cmd []string, vars variables, o *plugin.Options) error {
 	if len(cmd) != 2 {
 		return fmt.Errorf("expected len(cmd)==2, got %v", cmd)