WIP --- Update how we are loading executors #723
Conversation
fokion
force-pushed
the
feature/update-executor-cache
branch
2 times, most recently
from
0ea685c
to
f8f16f1
Compare
…ing them from the cache properly Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
fokion
force-pushed
the
feature/update-executor-cache
branch
from
f8f16f1
to
ff75472
Compare
|
CDS Report build-venom-a#105.0 ✘
|
There was a problem hiding this comment.
I'm not sure I understand the purpose of the pull-request, but if the purpose is not to read the file several times, there is already a cache on it.
If the goal is to have a cache on the executor once read with the input variables, the cache map would have to contain the input vars also in the key.
The following tests are failed:
case testsuite / testcase-case
testsuite with a user executor multilines / testA
testsuite.26115637 / test 2
testsuite with a user executor in custom dir / testfoobar custom
testsuite with a user executor in custom dir which has multiple steps / testfoobar multisteps custom
user_executor_http / call foobar_http_get_user
Many user executor / mytc
testsuite with a user executor.26115637 / Testy test part 1
play-with-array / mytc
play-with-array / using-var-from-arg
venom.go
Outdated
| } | ||
|
|
||
| v.RegisterExecutorUser(ux.Executor, ux) | ||
| ux.Input.AddAll(varsFromInput) | ||
| ux.Executor = executorName |
There was a problem hiding this comment.
I think there is no need to force assign here, ux.Executor already contains the value, as the name is already in the btes.
So, No need to have getExecutorName func.
venom.go
Outdated
| executorName, _ := getExecutorName(btes) | ||
| if len(executorName) == 0 { | ||
| fileName := filepath.Base(f) | ||
| executorName = strings.TrimSuffix(fileName, filepath.Ext(fileName)) |
There was a problem hiding this comment.
The name of user executor is the attribute in the file. It should not be defined by the filename.
In the read, we can see that the user executor is defined in the file lib/customA.yml, but the executor name is executor.
| @@ -44,6 +44,21 @@ func getVarFromPartialYML(ctx context.Context, btesIn []byte) (H, error) { | |||
| return partial.Vars, nil | |||
| } | |||
|
|
|||
| func getExecutorName(btes []byte) (string, error) { | |||
There was a problem hiding this comment.
Not needed, see comment below.
| @@ -229,12 +232,13 @@ func (v *Venom) RunUserExecutor(ctx context.Context, runner ExecutorRunner, tcIn | |||
| TestCaseInput: TestCaseInput{ | |||
| Name: ux.Executor, | |||
| RawTestSteps: ux.RawTestSteps, | |||
| Vars: vrs, | |||
| Vars: inputOnlyVrs, | |||
There was a problem hiding this comment.
tc.Vars == tc.TestCaseInput.Vars.
I don't understand the goal of the variable inputOnlyVrs: Doing TestCaseInput.Vars = intputOnlyVrs, and then tc.Vars.AddAll(vrs) is the same as the previous code.
| @@ -216,13 +215,19 @@ func (v *Venom) getUserExecutorFilesPath(vars map[string]string) (filePaths []st | |||
| } | |||
|
|
|||
| func (v *Venom) registerUserExecutors(ctx context.Context, name string, vars map[string]string) error { | |||
| _, ok := v.executorsUser[name] | |||
There was a problem hiding this comment.
We can't do a cache here. The GetExecutorRunner is used in RunUserExecutor, so that vars can contains different value depending of the input.
There was a problem hiding this comment.
Will mark it as WIP as I need to check if I have migrated all the necessary changes for it to work properly.
You do need to check if you have read the executor or not as this is the point of the cache , we want to read that file only once and not multiple times ) .
fokion
changed the title
…the executor from the file name if we dont define the executor field Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
check them if we have them in the cache and read them only once