Refactor to remove test args from remote test command

[tm-jdelapuente]

Context

plz test [TARGET] can take an argument to specify specific parts of a test target to run.

For example, plz test //[TARGET] -- [TestSubTarget] runs the subtarget function rather than all of the test functions in the target.

You can also specify what tests to run at a more granular level: e.g. [TestSubTarget/[NestedTestFunction]/[NestedTestFunction]

Problem
When specifying subtargets in plz test ... you get an error when you pass an argument that has a slash (e.g. plz test //[TARGET] -- TestSubTarget/MyTestFunction.

This shows up by running an additional test that errors, such as

tee: TestSubTarget/TestMyTestFunction/BulkGetMyResources: No such file or directory

Root cause

Please is constructing a cmd that calls tee and appends TestArgs at the end.

cmd, err := core.ReplaceTestSequences(state, target, target.GetTestCommand(state))
if len(state.TestArgs) != 0 {
	cmd += " " + strings.Join(state.TestArgs, " ")
}

This constructs a command like $TEST 2>&1 | tee $TMP_DIR/test.results [one or more subtests].

This is relevant because tee reads from stdin ($TEST 2>&1) and writes to stdout and files ($TMP_DIR/test.results [one or more subtests]).

Thus, this means that

• plz test //[TARGET] -- [TestSubTarget] creates and writes to the file TestSubTarget
• plz test //[TARGET] -- [TestSubTarget/[NestedTestFunction]/[NestedTestFunction] interprets the forward slashes as file paths and tries to create a file in said path but errors because the directory doesn't exist

Solution
The implication is that we need to avoid passing the subtests before the pipe to tee.

I thus removed the if statement to avoid passing the test arguments in the command.

Testing
I built Please with these changes and it worked as expected, only printing the subtests I passed and no erroring like it does now.

I'm concerned about any regressions and am not sure how to best test this.

Pending work
This only solves the problem on remote execution, not local execution.

[peterebden]

I think we pass this as an environment variable now, yes?

[tm-jdelapuente]

What do you mean? I don't think I'm following, because the above seems unrelated to that since it's about tee returning an error because it tries to write to a directory that doesn't exist

[peterebden]

The information about which sub-tests to run must be given to it somehow. IIRC it was originally an argument but then we changed it to an env var to avoid issues like this. I'm checking whether that does in fact happen as expected.

[tm-jdelapuente]

Just double checking, I understand you don't expect any further action from me while you check it?

[peterebden]

Yup ok we set one called TESTS which is read by the various test runners.
(In the case of the Go one at least, it isn't looking at arguments for it, you'd have to add -test.run as well for that to work)