Skip to content

testscript: support writing to stdio from inside a custom command#140

Closed
matthewmueller wants to merge 2 commits intorogpeppe:masterfrom
matthewmueller:command/stdio
Closed

testscript: support writing to stdio from inside a custom command#140
matthewmueller wants to merge 2 commits intorogpeppe:masterfrom
matthewmueller:command/stdio

Conversation

@matthewmueller
Copy link

@matthewmueller matthewmueller commented May 2, 2021

This PR allows you to write to stderr and stdout from within a custom command. For example,

func Test(t *testing.T) {
	p := testscript.Params{
		Cmds: map[string]func(ts *testscript.TestScript, neg bool, args []string){
			"render": render,
		},
	}
	testscript.Run(t, p)
}

func render(ts *testscript.TestScript, neg bool, args []string) {
    html, err := render.Render(ts.MkAbs(args))
    if err != nil {
      if neg {
        ts.Stdout("")
        ts.Stderr(err.Error())
        return
      }
      ts.Fatalf("Error rendering")
    }
    ts.Stdout(html)
    ts.Stderr("")
}

Allowing you to have a test like this:

render 'index.gohtml'
stdout '<h1>'
! stderr .

-- index.gohtml --
<html>
  <body>
    <h1>{{.}}</h1>
  </body>
</html>

And

! render 'index.gohtml'
stderr 'bad HTML'
! stdout .

-- index.gohtml --
<html

Closes #139

@matthewmueller matthewmueller changed the title testscript: support writing to stdio from inside a custom command. cl… testscript: support writing to stdio from inside a custom command May 2, 2021
@matthewmueller
Copy link
Author

matthewmueller commented May 2, 2021 •
edited
Loading

One thing that somewhat surprised me during testing was the need to do:

ts.Stderr("")

This was to make sure the previous command's stderr didn't linger. Another idea would be to adjust the API to:

ts.Stdio(stdout, stderr)

This would be slightly more cumbersome but ensure you don't forget to reset the other stream.

@matthewmueller matthewmueller mentioned this pull request May 2, 2021
Closed
@mvdan
Copy link
Collaborator

mvdan commented Jun 14, 2022

Sorry for the silence; this package is effectively maintained by three people, so sometimes none of us takes the initiative with new features like this :)

I agree that this is a limitation of the current API, and with @rogpeppe we've talked about this in the past. So I find it to be an entirely reasonable feature request to want to write to stdout - moving the command to the top level via RunMain is not a great solution, because it requires having a func TestMain (i.e. run as part of go test), and it also means running as a separate process.

As for the patch itself: first, I wonder if we need Stderr at all. Intuitively, I only want my builtins to write to stderr when they encounter an error and fail. You can already do this via https://pkg.go.dev/github.com/rogpeppe/go-internal/testscript#TestScript.Fatalf, and I argue that is a better API anyway - it also makes the builtin result in a failure status code.

I also think the behavior of Stdout might confuse some people. It replaces the entire output at each call, but it allow you to perform repeated calls and will happily log each one of those strings.

I think we have two options:

  1. Only allow up to one call to Stdout. Repeated calls result in a fatal error, as it's a bug in the user's code.

  2. Allow any number of calls, e.g. via one of these two APIs:

// easier to use for simple stuff and mirrors Fatalf, but no io.Writer
func (ts *TestScript) Printf(format string, args ...interface{})

// my personal preference:
// integrates with any io.Writer API nicely, and simple use cases can be fmt.Fprintf(ts.Stdout(), ...)
func (ts *TestScript) Stdout() io.Writer

@mvdan
Copy link
Collaborator

mvdan commented Jun 14, 2022

Also FYI @twpayne as we quite literally talked about this last week at the Go meetup in Zurich - how you can't move a top-level RunMain command to a builtin because builtins have no stdout.

@matthewmueller
Copy link
Author

matthewmueller commented Jun 14, 2022 •
edited
Loading

Hey, thanks for your response! I'm not actively using this fork anymore, so feel free to close.

I no longer have the right context to know if allowing up to one or multiple calls makes more sense. Intuitively, I think your second option makes more sense, but I don't want you to make API decisions based on hunches :)

@mvdan
Copy link
Collaborator

mvdan commented Jun 14, 2022

Thanks for your response! I'll leave this open for now then, to see if someone else needs this. I personally don't have a need for it, so I don't have an urge to get it done, but I also don't think we need to close the issue and PR right away.

@matthewmueller
Copy link
Author

matthewmueller commented Sep 15, 2022

Deciding to close this one to cleanup my open PRs view a bit. If someone needs this feel free to use this PR as a base!

@myitcv myitcv mentioned this pull request Apr 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow custom commands to write to stdout and stderr

2 participants

@matthewmueller @mvdan