Skip to content

Conversation

@rihter007
Copy link
Contributor

@rihter007 rihter007 commented Aug 9, 2022

Also the PR reveals variables usage in the new E2E test

@rihter007 rihter007 requested a review from mimir-d August 9, 2022 14:26
@rihter007 rihter007 force-pushed the feature/variables_teststep branch from 8a2a25d to c9d4fc5 Compare August 9, 2022 14:33
@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2022

Codecov Report

Merging #124 (e320523) into main (5b8e421) will decrease coverage by 6.36%.
The diff coverage is 5.40%.

@@            Coverage Diff             @@
##             main     #124      +/-   ##
==========================================
- Coverage   63.38%   57.01%   -6.37%     
==========================================
  Files         164      154      -10     
  Lines       10284     9985     -299     
==========================================
- Hits         6518     5693     -825     
- Misses       3045     3577     +532     
+ Partials      721      715       -6     
Flag Coverage Δ
e2e 49.04% <5.40%> (-0.19%) ⬇️
integration 54.60% <0.00%> (-0.06%) ⬇️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/runner/test_steps_variables.go 27.38% <0.00%> (-36.73%) ⬇️
plugins/teststeps/variables/variables.go 8.00% <8.00%> (ø)
pkg/remote/sync.go 0.00% <0.00%> (-94.12%) ⬇️
pkg/xcontext/metrics/prometheus/hacks.go 0.00% <0.00%> (-86.67%) ⬇️
pkg/xcontext/metrics/prometheus/count.go 0.00% <0.00%> (-84.62%) ⬇️
pkg/xcontext/logger/internal/wrap_printf.go 0.00% <0.00%> (-84.62%) ⬇️
pkg/xcontext/logger/internal/log_level.go 9.09% <0.00%> (-65.91%) ⬇️
pkg/xcontext/metrics/prometheus/metrics.go 23.60% <0.00%> (-57.61%) ⬇️
pkg/xcontext/metrics/prometheus/option.go 42.85% <0.00%> (-57.15%) ⬇️
pkg/job/job.go 20.40% <0.00%> (-55.11%) ⬇️
... and 45 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rihter007 rihter007 force-pushed the feature/variables_teststep branch 2 times, most recently from 9e5ab93 to 5a41e06 Compare August 9, 2022 15:03
return sva.AddRaw(tgtID, name, b)
}

func (sva *stepVariablesAccessor) AddRaw(tgtID string, name string, inRaw json.RawMessage) error {
Copy link
Member

Choose a reason for hiding this comment

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

you already have the Add(in interface{}); should be able to use that directly

if raw, ok := in.(json.RawMessage); ok {
    sva.tsv.Add(raw)
    return
}

@@ -0,0 +1,78 @@
package variables
Copy link
Member

Choose a reason for hiding this comment

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

please add docs md (see exec plugin for example))

}
}

func (ts *E2ETestSuite) startTask(taskFile string) types.JobID {
Copy link
Member

Choose a reason for hiding this comment

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

consistency in naming concepts

Suggested change
func (ts *E2ETestSuite) startTask(taskFile string) types.JobID {
func (ts *E2ETestSuite) startJob(descriptorFile string) types.JobID {

return teststeps.ForEachTarget(Name, ctx, ch, func(ctx xcontext.Context, target *target.Target) error {
for name, ps := range inputParams {
ctx.Debugf("add variable %s, value: %s", name, ps[0])
if err := stepsVars.AddRaw(target.ID, name, ps[0].RawMessage); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

so... whats the usage for this? renaming strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the plugin in general or that specific loop?

Copy link
Member

Choose a reason for hiding this comment

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

the plugin; i see it takes in strings, with possible interpolations, and outputs the result as a single string; this could be done just as well in the consumers of these output vars here. unless there's something im not seeing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, it is not a super-useful plugin. One can just directly put these strings in consuming plugins as you mentioned.
I made it because of three reasons:

  • it shows an example of how to use StepVariables and is also used in e2e test. ConTest has a number of "example" plugins like echo or randecho.
  • it allows to make constants that will be reused in multiple places. For example: if two or more plugins should consume the same parameter, one can use this plugin instead of copy-pasting.
  • on the future if step variables will have more options of being used (not just via parameters expanding) it could become a handy way to share variables

Add e2e TestVariables that checks step variables

Signed-off-by: Ilya <rihter007@inbox.ru>
@rihter007 rihter007 force-pushed the feature/variables_teststep branch from 5a41e06 to e320523 Compare August 22, 2022 15:09
@rihter007 rihter007 requested a review from mimir-d August 22, 2022 15:09
@mimir-d
Copy link
Member

mimir-d commented Aug 26, 2022

the added doc file makes it much more clear what the plugin is about

@rihter007 rihter007 merged commit 87e0e9f into main Aug 26, 2022
@rihter007 rihter007 deleted the feature/variables_teststep branch August 26, 2022 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants