internalize execute command

[kim-tsao]

Signed-off-by: Kim Tsao ktsao@redhat.com

What does this PR do?:

• Avoids exposing the execute command as a public method
• Ensures just a subset of commands (git and rm) are supported

Which issue(s)/story(ies) does this PR fixes:

https://issues.redhat.com/browse/DEVHAS-166

PR acceptance criteria:

• Unit/Functional tests

• Documentation

• Client Impact

How to test changes / Special notes to the reviewer:

[maysunfaisal]

What happens if someone forgets to do this in a future test? Will execute be the test execute in non-test code call hierarchy 🤔

[kim-tsao]

it is unnecessary to reset after every test as long as the mock function stays the same. I just did it for the sake of consistency and best practice (in case the mock function does change from test case to test case). In this particular test case, it was calling a mix of the original and mock execute functions, so the results were getting messed up. For non-test code, it should be the original execute since test functions are not exported

[maysunfaisal]

It seems like we're doing this in every test case..

I would just do this as a implementation of an interface once (can replace the Executor interface in gitops.go). So we can 2 implementation:

1. original execute
2. test execute

this will also solve the issue of not resetting execute at the end of every test 🤔

[kim-tsao]

This is a good suggestion (I don't like duplicate code either) but the limiting factor here is, the parameters to the mockExecute call varies. I can't pass in test arguments like outputStack, tt.errors, executedCmds to the interface function because that has to stay fixed in order for the override to work, so I have to inline the function declaration in each test in order to get the values of the test arguments that are set outside the scope of the execute function.

[maysunfaisal]

I think we could also put most of these in a util getter func? Does this work? But I leave it up to you

execute = newTestExecute(outputStack, tt.errors, executedCmds, baseDir, cmd, args...)

func newTestExecute(outputStack, tt.errors, executedCmds, baseDir, cmd, args...) func {
    return func(baseDir string, cmd CommandType, args ...string) ([]byte, error) {
				var output []byte
				var execErr error
				output, execErr, executedCmds = mockExecute(outputStack, tt.errors, executedCmds, baseDir, cmd, args...)
				return output, execErr
			}
}

[kim-tsao]

Yes, this does work. It's a lot cleaner so I will implement this. Thanks for the suggestions!

[keithchong]

So each test 'file' we have will have a similar mockExecute and newTestExecute ?

[kim-tsao]

So each test 'file' we have will have a similar mockExecute?

Only if you are testing a function that runs a git or rm execute command (and you want to avoid live calls). If the file is in the same package and the current mockExecute behaviour is sufficient, you can just call it, otherwise you will need to create your own.

[keithchong]

LGTM