Repository navigation
testscript/plugin: new package - #307
Conversation
| if v := os.Getenv(name); v != "" { | ||
| env.Vars = append(env.Vars, name+"="+v) | ||
| } | ||
| } |
There was a problem hiding this comment.
we removed SYSTEMROOT in 3030644, why are you re-adding these? note how os/exec always passes this one along now.
| // NeedStderr and NeedStdout indicate whether the command produces | ||
| // standard error or standard output respectively. | ||
| NeedStderr bool | ||
| NeedStdout bool |
There was a problem hiding this comment.
these never seem to be read by the non-test logic? either wire them properly or remove
There was a problem hiding this comment.
wired up and changed name to be more appropriate.
|
|
||
| // call sends a request and waits for its response, unmarshaling the result | ||
| // into result (if non-nil). A non-empty response error is returned as an error. | ||
| func (cl *client) call(method string, arg, result any) error { |
There was a problem hiding this comment.
my instinct is that this should take a context to be able to use timeouts/deadlines or cancel, as otherwise a plugin over RPC can hang or take a long time to reply, hanging the caller too.
There was a problem hiding this comment.
yeah, it can block indefinitely but currently there's no notion of context or timeout in command interface, so I think it's OK to just let it block for now, as with any other command implementation. We could add timeouts as an enhancement later.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cmd.Stderr = os.Stderr |
There was a problem hiding this comment.
note that if you send stderr straight to os.Stderr, any failures happening during go test will be mixed rather than being matched to the correct test run via e.g. testing.T.Log. perhaps not a big deal.
|
|
||
| // CWD is the absolute path of the current working directory the | ||
| // command should run in. | ||
| CWD string |
There was a problem hiding this comment.
I'd personally name this Dir, for consistency with os/exec and most other Go APIs
| // CmdParams holds the parameters for running a plugin command. | ||
| type CmdParams struct { | ||
| // CmdName is the name of the command being run. | ||
| CmdName string |
There was a problem hiding this comment.
nit: just Name? the other fields are not named CmdArgs etc.
|
|
||
| // Environ holds the values of the variables named in the command's | ||
| // [CmdInfo.RequiredEnv], as resolved in the test environment. | ||
| Environ map[string]string |
There was a problem hiding this comment.
the name Environ in Go makes me think of os.Environ, which is a []string, not a map. so if you really do want to use a map, I'd use a different name like Env
|
|
||
| // Stdin holds the standard input for the command. | ||
| Stdin []byte | ||
| } |
There was a problem hiding this comment.
it looks like this plugin system doesn't support background commands. that's probably fine for the RPC model, but do we test that these are rejected properly?
There was a problem hiding this comment.
I'm not sure there's much need: & is only supported for exec commands not commands in general.
| } | ||
|
|
||
| // PluginInfo holds static information about a plugin. | ||
| type PluginInfo struct { |
There was a problem hiding this comment.
personally I'd add something like a Version int here, starting at 1, such that we can later bump this if we need to do any significant changes.
There was a problem hiding this comment.
done, a bit differently, allowing for version negotiation.
2c7a5ac to
7f763ef
Compare
This adds the capability for a testscript script to use a "plugin", an external binary that implements support for additional functionality not easily implemented with a regular command. Except when using the testscript command, it must be explicitly enabled by using `plugin.Setup`. A plugin binary is run as an RPC server communicating over stdin/stdout, and can provide extra commands and environment variables to a testscript test. A classic use case is to run a scratch server, set an environment variable referring to its address, and tear it down when a test is done. For now we use a custom RPC implementation so we can avoid polluting the Go API (unfortunately `net/rpc` requires that the types used are made public). When `plugin` is invoked inside a testscript, e.g. ``` plugin foo ``` a plugin binary is searched for with the name `testscript-plugin-foo`.
This adds the capability for a testscript script to use a "plugin", an external binary that implements support for additional functionality not easily implemented with a regular command. Except when using the testscript command, it must be explicitly enabled by using
plugin.Setup.A plugin binary is run as an RPC server communicating over stdin/stdout, and can provide extra commands and environment variables to a testscript test. A classic use case is to run a scratch server, set an environment variable referring to its address, and tear it down when a test is done.
For now we use a custom RPC implementation so we can avoid polluting the Go API (unfortunately
net/rpcrequires that the types used are made public).When
pluginis invoked inside a testscript, e.g.a plugin binary is searched for with the name
testscript-plugin-foo.For reference, we implement the existing Go proxy support as a plugin. In the future we may decide to fully factor out this support from the core testscript executable, although we'll have to be careful about backward compatibility concerns.