diff --git a/pkg/pluginregistry/bundles.go b/pkg/pluginregistry/bundles.go index 9457bd57..6c81cd66 100644 --- a/pkg/pluginregistry/bundles.go +++ b/pkg/pluginregistry/bundles.go @@ -30,8 +30,8 @@ func (r *PluginRegistry) NewTestStepBundle(ctx xcontext.Context, testStepDescrip if len(label) == 0 { return nil, ErrStepLabelIsMandatory{TestStepDescriptor: testStepDescriptor} } - if err := test.CheckVariableName(label); err != nil { - return nil, InvalidVariableFormat{InvalidName: label, Err: err} + if err := test.CheckIdentifier(label); err != nil { + return nil, ErrInvalidStepLabelFormat{InvalidName: label, Err: err} } testStepBundle := test.TestStepBundle{ diff --git a/pkg/pluginregistry/errors.go b/pkg/pluginregistry/errors.go index bc71b181..e169e593 100644 --- a/pkg/pluginregistry/errors.go +++ b/pkg/pluginregistry/errors.go @@ -19,16 +19,16 @@ func (err ErrStepLabelIsMandatory) Error() string { return fmt.Sprintf("step has no label, but it is mandatory (step: %+v)", err.TestStepDescriptor) } -// InvalidVariableFormat tells that a variable name doesn't fit the variable name format (alphanum + '_') -type InvalidVariableFormat struct { +// ErrInvalidStepLabelFormat tells that a variable name doesn't fit the variable name format (alphanum + '_') +type ErrInvalidStepLabelFormat struct { InvalidName string Err error } -func (err InvalidVariableFormat) Error() string { +func (err ErrInvalidStepLabelFormat) Error() string { return fmt.Sprintf("'%s' doesn't match variable name format: %v", err.InvalidName, err.Err) } -func (err InvalidVariableFormat) Unwrap() error { +func (err ErrInvalidStepLabelFormat) Unwrap() error { return err.Err } diff --git a/pkg/runner/test_runner.go b/pkg/runner/test_runner.go index 2ba47bfc..dedad1db 100644 --- a/pkg/runner/test_runner.go +++ b/pkg/runner/test_runner.go @@ -44,7 +44,7 @@ type TestRunner struct { steps []*stepState // The pipeline, in order of execution targets map[string]*targetState // Target state lookup map - stepsVariables *testStepsVariables + stepOutputs *testStepsVariables // contains emitted steps variables // One mutex to rule them all, used to serialize access to all the state above. // Could probably be split into several if necessary. @@ -171,14 +171,14 @@ func (tr *TestRunner) Run( } var err error - tr.stepsVariables, err = newTestStepsVariables(t.TestStepsBundles) + tr.stepOutputs, err = newTestStepsVariables(t.TestStepsBundles) if err != nil { ctx.Errorf("Failed to initialise test steps variables: %v", err) return nil, nil, err } for targetID, targetState := range tr.targets { - if err := tr.stepsVariables.initTargetStepsVariables(targetID, targetState.StepsVariables); err != nil { + if err := tr.stepOutputs.initTargetStepsVariables(targetID, targetState.StepsVariables); err != nil { ctx.Errorf("Failed to initialise test steps variables for target: %s: %v", targetID, err) return nil, nil, err } @@ -258,7 +258,7 @@ func (tr *TestRunner) Run( numInFlightTargets := 0 for i, tgt := range targets { tgs := tr.targets[tgt.ID] - tgs.StepsVariables, err = tr.stepsVariables.getTargetStepsVariables(tgt.ID) + tgs.StepsVariables, err = tr.stepOutputs.getTargetStepsVariables(tgt.ID) if err != nil { ctx.Errorf("Failed to get steps variables: %v", err) return nil, nil, err @@ -545,7 +545,7 @@ func (tr *TestRunner) runStepIfNeeded(ss *stepState) error { resumeState := ss.resumeState ss.resumeState = nil - sva := newStepVariablesAccessor(ss.sb.TestStepLabel, tr.stepsVariables) + sva := newStepVariablesAccessor(ss.sb.TestStepLabel, tr.stepOutputs) resultCh, addTarget, err := ss.stepRunner.Run(ss.ctx, ss.sb, sva, ss.ev, resumeState, ss.resumeStateTargets) if err != nil { return fmt.Errorf("failed to stert a step runner for '%s': %v", ss.sb.TestStepLabel, err) diff --git a/pkg/runner/test_runner_test.go b/pkg/runner/test_runner_test.go index 7584b0dd..77721047 100644 --- a/pkg/runner/test_runner_test.go +++ b/pkg/runner/test_runner_test.go @@ -471,7 +471,7 @@ func (s *TestRunnerSuite) TestRandomizedMultiStep() { } func (s *TestRunnerSuite) TestVariables() { - ctx, cancel := xcontext.WithCancel(logrusctx.NewContext(logger.LevelDebug)) + ctx, cancel := logrusctx.NewContext(logger.LevelDebug) defer cancel() var ( diff --git a/pkg/test/step.go b/pkg/test/step.go index d4e20cae..c3456d3a 100644 --- a/pkg/test/step.go +++ b/pkg/test/step.go @@ -9,12 +9,13 @@ import ( "encoding/json" "errors" "fmt" + "regexp" + "strconv" + "github.com/linuxboot/contest/pkg/event" "github.com/linuxboot/contest/pkg/event/testevent" "github.com/linuxboot/contest/pkg/target" "github.com/linuxboot/contest/pkg/xcontext" - "strconv" - "unicode" ) // TestStepParameters represents the parameters that a TestStep should consume @@ -109,6 +110,14 @@ type TestStepChannels struct { } // StepsVariables represents a read/write access for step variables +// Example: +// var sv StepsVariables +// intVar := 42 +// sv.Add("dummy-target-id", "varname", intVar) +// +// var recvIntVar int +// sv.Get(("dummy-target-id", "varname", &recvIntVar) +// assert recvIntVar == 42 type StepsVariables interface { // Add adds a new or replaces existing variable associated with current test step and target Add(tgtID string, name string, in interface{}) error @@ -131,21 +140,24 @@ type TestStep interface { ValidateParameters(ctx xcontext.Context, params TestStepParameters) error } -// CheckVariableName checks that input argument forms a valid variable name -func CheckVariableName(s string) error { +var identifierRegexPattern *regexp.Regexp + +func init() { + var err error + identifierRegexPattern, err = regexp.Compile(`[a-zA-Z]\w*`) + if err != nil { + panic(fmt.Sprintf("invalid identifier matching regexp expression: %v", err)) + } +} + +// CheckIdentifier checks that input argument forms a valid identifier name +func CheckIdentifier(s string) error { if len(s) == 0 { - return fmt.Errorf("empty variable name") + return fmt.Errorf("empty identifier") } - for idx, ch := range s { - if idx == 0 { - if !unicode.IsLetter(ch) { - return fmt.Errorf("first character should be alpha, got %c", ch) - } - } - if unicode.IsLetter(ch) || unicode.IsDigit(ch) || ch == '_' { - continue - } - return fmt.Errorf("got unxpected character: %c", ch) + match := identifierRegexPattern.FindString(s) + if match != s { + return fmt.Errorf("identifier should be an non-empty string that matches: %s", identifierRegexPattern.String()) } return nil } diff --git a/pkg/test/step_test.go b/pkg/test/step_test.go index c0b1fe31..be546f72 100644 --- a/pkg/test/step_test.go +++ b/pkg/test/step_test.go @@ -49,9 +49,9 @@ func TestTestStepParametersUnmarshalNested(t *testing.T) { } func TestCheckVariableName(t *testing.T) { - require.NoError(t, CheckVariableName("Abc123_XYZ")) - require.Error(t, CheckVariableName("")) - require.Error(t, CheckVariableName("1AAA")) - require.Error(t, CheckVariableName("a b")) - require.Error(t, CheckVariableName("a()+b")) + require.NoError(t, CheckIdentifier("Abc123_XYZ")) + require.Error(t, CheckIdentifier("")) + require.Error(t, CheckIdentifier("1AAA")) + require.Error(t, CheckIdentifier("a b")) + require.Error(t, CheckIdentifier("a()+b")) }