Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP --- Update how we are loading executors #723

Closed
wants to merge 2 commits into from
Closed

WIP --- Update how we are loading executors #723

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff75472
Fix how we are reading the executors as at the moment we are not load…
fokion Sep 27, 2023
183f74e
add tests to the codebase , remove the suggestion to get the name of …
fokion Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions process_teststep.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ func (v *Venom) RunTestStep(ctx context.Context, e ExecutorRunner, tc *TestCase,
if oDir == "" {
oDir = "."
}

filename := path.Join(oDir, fmt.Sprintf("%s.%s.step.%d.%d.dump.json", slug.Make(StringVarFromCtx(ctx, "venom.testsuite.shortName")), slug.Make(tc.Name), stepNumber, rangedIndex))

if err := os.WriteFile(filename, []byte(HideSensitive(ctx, string(output))), 0644); err != nil {
Expand Down
15 changes: 15 additions & 0 deletions read_partial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,21 @@ func getVarFromPartialYML(ctx context.Context, btesIn []byte) (H, error) {
return partial.Vars, nil
}

func getExecutorName(btes []byte) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, see comment below.

content := readPartialYML(btes, "executor")
type partialType struct {
Executor string `yaml:"executor" json:"executor"`
}
partial := &partialType{}
if len(content) > 0 {
if err := yaml.Unmarshal([]byte(content), &partial); err != nil {
Error(context.Background(), "file content: %s", string(btes))
return "", errors.Wrapf(err, "error while unmarshal - see venom.log")
}
}
return partial.Executor, nil
}

// readPartialYML extract a yml part from a given string
func readPartialYML(btes []byte, attribute string) string {
var result []string
Expand Down
8 changes: 6 additions & 2 deletions types_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,19 +203,22 @@ func (ux UserExecutor) ZeroValueResult() interface{} {
func (v *Venom) RunUserExecutor(ctx context.Context, runner ExecutorRunner, tcIn *TestCase, tsIn *TestStepResult, step TestStep) (interface{}, error) {
vrs := tcIn.TestSuiteVars.Clone()
uxIn := runner.GetExecutor().(UserExecutor)

inputOnlyVrs := H{}
for k, va := range uxIn.Input {
if strings.HasPrefix(k, "input.") {
// do not reinject input.vars from parent user executor if exists
continue
} else if !strings.HasPrefix(k, "venom") {
if vl, ok := step[k]; ok && vl != "" { // value from step
vrs.AddWithPrefix("input", k, vl)
inputOnlyVrs.AddWithPrefix("input", k, vl)
} else { // default value from executor
vrs.AddWithPrefix("input", k, va)
inputOnlyVrs.AddWithPrefix("input", k, va)
}
} else {
vrs.Add(k, va)
inputOnlyVrs.Add(k, va)
}
}
// reload the user executor with the interpolated vars
Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
TestSuiteVars: tcIn.TestSuiteVars,
IsExecutor: true,
TestStepResults: make([]TestStepResult, 0),
}
tc.Vars.AddAll(vrs)

tc.originalName = tc.Name
tc.Name = slug.Make(tc.Name)
Expand Down
54 changes: 17 additions & 37 deletions venom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (

"github.com/confluentinc/bincover"
"github.com/fatih/color"
"github.com/ovh/cds/sdk/interpolate"
"github.com/pkg/errors"
"github.com/rockbears/yaml"
"github.com/spf13/cast"
Expand Down Expand Up @@ -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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do a cache here. The GetExecutorRunner is used in RunUserExecutor, so that vars can contains different value depending of the input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ) .

if ok {
return nil
}
if v.executorFileCache != nil && len(v.executorFileCache) != 0 {
return errors.Errorf("Could not find executor with name %v ", name)
}
executorsPath, err := v.getUserExecutorFilesPath(vars)
if err != nil {
return err
}

for _, f := range executorsPath {
Info(ctx, "Reading %v", f)
btes, ok := v.executorFileCache[f]
if !ok {
btes, err = os.ReadFile(f)
Expand All @@ -231,49 +236,24 @@ func (v *Venom) registerUserExecutors(ctx context.Context, name string, vars map
}
v.executorFileCache[f] = btes
}

varsFromInput, err := getUserExecutorInputYML(ctx, btes)
if err != nil {
return err
}

// varsFromInput contains the default vars from the executor
var varsFromInputMap map[string]string
if len(varsFromInput) > 0 {
varsFromInputMap, err = DumpStringPreserveCase(varsFromInput)
if err != nil {
return errors.Wrapf(err, "unable to parse variables")
}
executorName, _ := getExecutorName(btes)
if len(executorName) == 0 {
fileName := filepath.Base(f)
executorName = strings.TrimSuffix(fileName, filepath.Ext(fileName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

varsComputed := map[string]string{}
for k, v := range vars {
varsComputed[k] = v
}
for k, v := range varsFromInputMap {
// we only take vars from varsFromInputMap if it's not already exist in vars from teststep vars
if _, ok := vars[k]; !ok {
varsComputed[k] = v
}
}

content, err := interpolate.Do(string(btes), varsComputed)
varsFromInput, err := getUserExecutorInputYML(ctx, btes)
if err != nil {
return err
}

ux := UserExecutor{Filename: f}
if err := yaml.Unmarshal([]byte(content), &ux); err != nil {
return errors.Wrapf(err, "unable to parse file %q with content %v", f, content)
}

Debug(ctx, "User executor %q revolved with content %v", f, content)

for k, vr := range varsComputed {
ux.Input.Add(k, vr)
if err := yaml.Unmarshal(btes, &ux); err != nil {
return errors.Wrapf(err, "unable to parse file %q", f)
}

v.RegisterExecutorUser(ux.Executor, ux)
ux.Input.AddAll(varsFromInput)
ux.Executor = executorName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

v.RegisterExecutorUser(executorName, ux)
}
return nil
}
Expand Down