From 8ba3306aa4ad9bf9b3c0b0b1506074f6de673fa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torbj=C3=B8rn=20Vatn?= Date: Tue, 17 Nov 2020 18:31:05 +0100 Subject: [PATCH] EvalBool and Interpolation fixes (#424) * A new attempt at Interpolation and EvalBool * One small merge fix * Remove some fmt.Printfs --- .github/workflows/test-expressions.yml | 82 ++++++ .github/workflows/test-if.yml | 391 +++++++++++++++++++++++++ pkg/runner/expression.go | 38 ++- pkg/runner/expression_test.go | 81 ++++- pkg/runner/run_context.go | 95 +++--- pkg/runner/run_context_test.go | 192 ++++++++---- 6 files changed, 762 insertions(+), 117 deletions(-) create mode 100644 .github/workflows/test-expressions.yml create mode 100644 .github/workflows/test-if.yml diff --git a/.github/workflows/test-expressions.yml b/.github/workflows/test-expressions.yml new file mode 100644 index 0000000..ec1ee43 --- /dev/null +++ b/.github/workflows/test-expressions.yml @@ -0,0 +1,82 @@ + +name: "Test how expressions are handled on Github" +on: push + +env: + KEYWITHNOTHING: valuewithnothing + KEY-WITH-HYPHENS: value-with-hyphens + KEY_WITH_UNDERSCORES: value_with_underscores + SOMETHING_TRUE: true + SOMETHING_FALSE: false + + +jobs: + test-espressions: + runs-on: ubuntu-latest + steps: + + - name: €{{ 1 }} to €{{ 2 }} -> ${{1}} to ${{2}} should be equal to 1 to 2 + run: echo "Done " + + - name: €{{ env.KEYWITHNOTHING }} -> ${{ env.KEYWITHNOTHING }} should be equal to valuewithnothing + run: echo "Done " + + - name: €{{ env.KEY-WITH-HYPHENS }} -> ${{ env.KEY-WITH-HYPHENS }} should be equal to value-with-hyphens + run: echo "Done " + + - name: €{{ env.KEY_WITH_UNDERSCORES }} -> ${{ env.KEY_WITH_UNDERSCORES }} should be equal to value_with_underscores + run: echo "Done " + + - name: €{{ env.UNKNOWN }} -> ${{ env.UNKNOWN }} should be equal to + run: echo "Done " + + - name: €{{ env.SOMETHING_TRUE }} -> ${{ env.SOMETHING_TRUE }} should be equal to true + run: echo "Done " + + - name: €{{ env.SOMETHING_FALSE }} -> ${{ env.SOMETHING_FALSE }} should be equal to false + run: echo "Done " + + - name: €{{ !env.SOMETHING_TRUE }} -> ${{ !env.SOMETHING_TRUE }} should be equal to false + run: echo "Done " + + - name: €{{ !env.SOMETHING_FALSE }} -> ${{ !env.SOMETHING_FALSE }} should be equal to false + run: echo "Done " + + - name: €{{ !env.SOMETHING_TRUE && true }} -> ${{ !env.SOMETHING_TRUE && true }} should be equal to false + run: echo "Done " + + - name: €{{ !env.SOMETHING_FALSE && true }} -> ${{ !env.SOMETHING_FALSE && true }} should be equal to false + run: echo "Done " + + - name: €{{ env.SOMETHING_TRUE && true }} -> ${{ env.SOMETHING_TRUE && true }} should be equal to true + run: echo "Done " + + - name: €{{ env.SOMETHING_FALSE && true }} -> ${{ env.SOMETHING_FALSE && true }} should be equal to true + run: echo "Done " + + - name: €{{ !env.SOMETHING_TRUE || true }} -> ${{ !env.SOMETHING_TRUE || true }} should be equal to true + run: echo "Done " + + - name: €{{ !env.SOMETHING_FALSE || true }} -> ${{ !env.SOMETHING_FALSE || true }} should be equal to true + run: echo "Done " + + - name: €{{ !env.SOMETHING_TRUE && false }} -> ${{ !env.SOMETHING_TRUE && false }} should be equal to false + run: echo "Done " + + - name: €{{ !env.SOMETHING_FALSE && false }} -> ${{ !env.SOMETHING_FALSE && false }} should be equal to false + run: echo "Done " + + - name: €{{ !env.SOMETHING_TRUE || false }} -> ${{ !env.SOMETHING_TRUE || false }} should be equal to false + run: echo "Done " + + - name: €{{ !env.SOMETHING_FALSE || false }} -> ${{ !env.SOMETHING_FALSE || false }} should be equal to false + run: echo "Done " + + - name: €{{ env.SOMETHING_TRUE || false }} -> ${{ env.SOMETHING_TRUE || false }} should be equal to true + run: echo "Done " + + - name: €{{ env.SOMETHING_FALSE || false }} -> ${{ env.SOMETHING_FALSE || false }} should be equal to false + run: echo "Done " + + - name: €{{ env.SOMETHING_FALSE }} && €{{ env.SOMETHING_TRUE }} -> ${{ env.SOMETHING_FALSE }} && ${{ env.SOMETHING_TRUE }} should be equal to false && true + run: echo "Done " diff --git a/.github/workflows/test-if.yml b/.github/workflows/test-if.yml new file mode 100644 index 0000000..191e7c2 --- /dev/null +++ b/.github/workflows/test-if.yml @@ -0,0 +1,391 @@ + +name: "Test what expressions result in true and false on Github" +on: push + +env: + SOMETHING_TRUE: true + SOMETHING_FALSE: false + SOME_TEXT: text + + +jobs: + test-ifs-and-buts: + runs-on: ubuntu-latest + steps: + + - name: "❌ I should not run, expr: failure()" + id: step0 + if: failure() + run: echo "failure() should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: success()" + id: step1 + if: success() + run: echo OK + + - name: "Double checking expr: success()" + if: steps.step1.conclusion == 'skipped' + run: echo "success() should have been true, but wasn't" + + - name: "❌ I should not run, expr: cancelled()" + id: step2 + if: cancelled() + run: echo "cancelled() should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: always()" + id: step3 + if: always() + run: echo OK + + - name: "Double checking expr: always()" + if: steps.step3.conclusion == 'skipped' + run: echo "always() should have been true, but wasn't" + + - name: "✅ I should run, expr: true" + id: step4 + if: true + run: echo OK + + - name: "Double checking expr: true" + if: steps.step4.conclusion == 'skipped' + run: echo "true should have been true, but wasn't" + + - name: "❌ I should not run, expr: false" + id: step5 + if: false + run: echo "false should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: 1 != 0" + id: step8 + if: 1 != 0 + run: echo OK + + - name: "Double checking expr: 1 != 0" + if: steps.step8.conclusion == 'skipped' + run: echo "1 != 0 should have been true, but wasn't" + + - name: "❌ I should not run, expr: 1 != 1" + id: step9 + if: 1 != 1 + run: echo "1 != 1 should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: €{{ 1 != 0 }}" + id: step10 + if: ${{ 1 != 0 }} + run: echo OK + + - name: "Double checking expr: €{{ 1 != 0 }}" + if: steps.step10.conclusion == 'skipped' + run: echo "${{ 1 != 0 }} should have been true, but wasn't" + + - name: "❌ I should not run, expr: €{{ 1 != 1 }}" + id: step11 + if: ${{ 1 != 1 }} + run: echo "${{ 1 != 1 }} should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: 1 == 0" + id: step12 + if: 1 == 0 + run: echo "1 == 0 should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: 1 == 1" + id: step13 + if: 1 == 1 + run: echo OK + + - name: "Double checking expr: 1 == 1" + if: steps.step13.conclusion == 'skipped' + run: echo "1 == 1 should have been true, but wasn't" + + - name: "❌ I should not run, expr: 1 > 2" + id: step14 + if: 1 > 2 + run: echo "1 > 2 should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: 1 < 2" + id: step15 + if: 1 < 2 + run: echo OK + + - name: "Double checking expr: 1 < 2" + if: steps.step15.conclusion == 'skipped' + run: echo "1 < 2 should have been true, but wasn't" + + - name: "❌ I should not run, expr: true && false" + id: step16 + if: true && false + run: echo "true && false should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: true && 1 < 2" + id: step17 + if: true && 1 < 2 + run: echo OK + + - name: "Double checking expr: true && 1 < 2" + if: steps.step17.conclusion == 'skipped' + run: echo "true && 1 < 2 should have been true, but wasn't" + + - name: "✅ I should run, expr: false || 1 < 2" + id: step18 + if: false || 1 < 2 + run: echo OK + + - name: "Double checking expr: false || 1 < 2" + if: steps.step18.conclusion == 'skipped' + run: echo "false || 1 < 2 should have been true, but wasn't" + + - name: "❌ I should not run, expr: false || false" + id: step19 + if: false || false + run: echo "false || false should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: env.UNKNOWN == 'true'" + id: step20 + if: env.UNKNOWN == 'true' + run: echo "env.UNKNOWN == 'true' should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: env.UNKNOWN" + id: step21 + if: env.UNKNOWN + run: echo "env.UNKNOWN should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: env.SOME_TEXT" + id: step22 + if: env.SOME_TEXT + run: echo OK + + - name: "Double checking expr: env.SOME_TEXT" + if: steps.step22.conclusion == 'skipped' + run: echo "env.SOME_TEXT should have been true, but wasn't" + + - name: "✅ I should run, expr: env.SOME_TEXT == 'text'" + id: step23 + if: env.SOME_TEXT == 'text' + run: echo OK + + - name: "Double checking expr: env.SOME_TEXT == 'text'" + if: steps.step23.conclusion == 'skipped' + run: echo "env.SOME_TEXT == 'text' should have been true, but wasn't" + + - name: "✅ I should run, expr: env.SOMETHING_TRUE == 'true'" + id: step24 + if: env.SOMETHING_TRUE == 'true' + run: echo OK + + - name: "Double checking expr: env.SOMETHING_TRUE == 'true'" + if: steps.step24.conclusion == 'skipped' + run: echo "env.SOMETHING_TRUE == 'true' should have been true, but wasn't" + + - name: "❌ I should not run, expr: env.SOMETHING_FALSE == 'true'" + id: step25 + if: env.SOMETHING_FALSE == 'true' + run: echo "env.SOMETHING_FALSE == 'true' should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: env.SOMETHING_TRUE" + id: step26 + if: env.SOMETHING_TRUE + run: echo OK + + - name: "Double checking expr: env.SOMETHING_TRUE" + if: steps.step26.conclusion == 'skipped' + run: echo "env.SOMETHING_TRUE should have been true, but wasn't" + + - name: "✅ I should run, expr: env.SOMETHING_FALSE" + id: step27 + if: env.SOMETHING_FALSE + run: echo OK + + - name: "Double checking expr: env.SOMETHING_FALSE" + if: steps.step27.conclusion == 'skipped' + run: echo "env.SOMETHING_FALSE should have been true, but wasn't" + + - name: "❌ I should not run, expr: €{{ !env.SOMETHING_TRUE }}" + id: step30 + if: ${{ !env.SOMETHING_TRUE }} + run: echo "${{ !env.SOMETHING_TRUE }} should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: €{{ !env.SOMETHING_FALSE }}" + id: step31 + if: ${{ !env.SOMETHING_FALSE }} + run: echo "${{ !env.SOMETHING_FALSE }} should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: €{{ ! env.SOMETHING_TRUE }}" + id: step32 + if: ${{ ! env.SOMETHING_TRUE }} + run: echo "${{ ! env.SOMETHING_TRUE }} should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: €{{ ! env.SOMETHING_FALSE }}" + id: step33 + if: ${{ ! env.SOMETHING_FALSE }} + run: echo "${{ ! env.SOMETHING_FALSE }} should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: €{{ env.SOMETHING_TRUE }}" + id: step34 + if: ${{ env.SOMETHING_TRUE }} + run: echo OK + + - name: "Double checking expr: €{{ env.SOMETHING_TRUE }}" + if: steps.step34.conclusion == 'skipped' + run: echo "${{ env.SOMETHING_TRUE }} should have been true, but wasn't" + + - name: "✅ I should run, expr: €{{ env.SOMETHING_FALSE }}" + id: step35 + if: ${{ env.SOMETHING_FALSE }} + run: echo OK + + - name: "Double checking expr: €{{ env.SOMETHING_FALSE }}" + if: steps.step35.conclusion == 'skipped' + run: echo "${{ env.SOMETHING_FALSE }} should have been true, but wasn't" + + - name: "❌ I should not run, expr: €{{ !env.SOMETHING_TRUE }}" + id: step36 + if: ${{ !env.SOMETHING_TRUE }} + run: echo "${{ !env.SOMETHING_TRUE }} should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: €{{ !env.SOMETHING_FALSE }}" + id: step37 + if: ${{ !env.SOMETHING_FALSE }} + run: echo "${{ !env.SOMETHING_FALSE }} should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: €{{ !env.SOMETHING_TRUE && true }}" + id: step38 + if: ${{ !env.SOMETHING_TRUE && true }} + run: echo "${{ !env.SOMETHING_TRUE && true }} should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: €{{ !env.SOMETHING_FALSE && true }}" + id: step39 + if: ${{ !env.SOMETHING_FALSE && true }} + run: echo "${{ !env.SOMETHING_FALSE && true }} should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: €{{ !env.SOMETHING_TRUE || true }}" + id: step40 + if: ${{ !env.SOMETHING_TRUE || true }} + run: echo OK + + - name: "Double checking expr: €{{ !env.SOMETHING_TRUE || true }}" + if: steps.step40.conclusion == 'skipped' + run: echo "${{ !env.SOMETHING_TRUE || true }} should have been true, but wasn't" + + - name: "❌ I should not run, expr: €{{ !env.SOMETHING_FALSE || false }}" + id: step41 + if: ${{ !env.SOMETHING_FALSE || false }} + run: echo "${{ !env.SOMETHING_FALSE || false }} should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: €{{ env.SOMETHING_TRUE && true }}" + id: step42 + if: ${{ env.SOMETHING_TRUE && true }} + run: echo OK + + - name: "Double checking expr: €{{ env.SOMETHING_TRUE && true }}" + if: steps.step42.conclusion == 'skipped' + run: echo "${{ env.SOMETHING_TRUE && true }} should have been true, but wasn't" + + - name: "✅ I should run, expr: €{{ env.SOMETHING_FALSE || true }}" + id: step43 + if: ${{ env.SOMETHING_FALSE || true }} + run: echo OK + + - name: "Double checking expr: €{{ env.SOMETHING_FALSE || true }}" + if: steps.step43.conclusion == 'skipped' + run: echo "${{ env.SOMETHING_FALSE || true }} should have been true, but wasn't" + + - name: "✅ I should run, expr: €{{ env.SOMETHING_FALSE || false }}" + id: step44 + if: ${{ env.SOMETHING_FALSE || false }} + run: echo OK + + - name: "Double checking expr: €{{ env.SOMETHING_FALSE || false }}" + if: steps.step44.conclusion == 'skipped' + run: echo "${{ env.SOMETHING_FALSE || false }} should have been true, but wasn't" + + - name: "✅ I should run, expr: €{{ env.SOMETHING_TRUE == 'true' }}" + id: step46 + if: ${{ env.SOMETHING_TRUE == 'true'}} + run: echo OK + + - name: "Double checking expr: €{{ env.SOMETHING_TRUE == 'true' }}" + if: steps.step46.conclusion == 'skipped' + run: echo "${{ env.SOMETHING_TRUE == 'true'}} should have been true, but wasn't" + + - name: "❌ I should not run, expr: €{{ env.SOMETHING_FALSE == 'true' }}" + id: step47 + if: ${{ env.SOMETHING_FALSE == 'true'}} + run: echo "${{ env.SOMETHING_FALSE == 'true'}} should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: €{{ env.SOMETHING_FALSE == 'false' }}" + id: step48 + if: ${{ env.SOMETHING_FALSE == 'false'}} + run: echo OK + + - name: "Double checking expr: €{{ env.SOMETHING_FALSE == 'false' }}" + if: steps.step48.conclusion == 'skipped' + run: echo "${{ env.SOMETHING_FALSE == 'false'}} should have been true, but wasn't" + + - name: "✅ I should run, expr: €{{ env.SOMETHING_FALSE }} && €{{ env.SOMETHING_TRUE }}" + id: step49 + if: ${{ env.SOMETHING_FALSE }} && ${{ env.SOMETHING_TRUE }} + run: echo OK + + - name: "Double checking expr: €{{ env.SOMETHING_FALSE }} && €{{ env.SOMETHING_TRUE }}" + if: steps.step49.conclusion == 'skipped' + run: echo "${{ env.SOMETHING_FALSE }} && ${{ env.SOMETHING_TRUE }} should have been true, but wasn't" + + - name: "✅ I should run, expr: false || env.SOMETHING_TRUE == 'true'" + id: step50 + if: false || env.SOMETHING_TRUE == 'true' + run: echo OK + + - name: "Double checking expr: false || env.SOMETHING_TRUE == 'true'" + if: steps.step50.conclusion == 'skipped' + run: echo "false || env.SOMETHING_TRUE == 'true' should have been true, but wasn't" + + - name: "✅ I should run, expr: true || env.SOMETHING_FALSE == 'true'" + id: step51 + if: true || env.SOMETHING_FALSE == 'true' + run: echo OK + + - name: "Double checking expr: true || env.SOMETHING_FALSE == 'true'" + if: steps.step51.conclusion == 'skipped' + run: echo "true || env.SOMETHING_FALSE == 'true' should have been true, but wasn't" + + - name: "✅ I should run, expr: true && env.SOMETHING_TRUE == 'true'" + id: step52 + if: true && env.SOMETHING_TRUE == 'true' + run: echo OK + + - name: "Double checking expr: true && env.SOMETHING_TRUE == 'true'" + if: steps.step52.conclusion == 'skipped' + run: echo "true && env.SOMETHING_TRUE == 'true' should have been true, but wasn't" + + - name: "❌ I should not run, expr: false && env.SOMETHING_TRUE == 'true'" + id: step53 + if: false && env.SOMETHING_TRUE == 'true' + run: echo "false && env.SOMETHING_TRUE == 'true' should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: env.SOMETHING_FALSE == 'true' && env.SOMETHING_TRUE == 'true'" + id: step54 + if: env.SOMETHING_FALSE == 'true' && env.SOMETHING_TRUE == 'true' + run: echo "env.SOMETHING_FALSE == 'true' && env.SOMETHING_TRUE == 'true' should be false, but was evaluated to true;" exit 1; + + - name: "❌ I should not run, expr: env.SOMETHING_FALSE == 'true' && true" + id: step55 + if: env.SOMETHING_FALSE == 'true' && true + run: echo "env.SOMETHING_FALSE == 'true' && true should be false, but was evaluated to true;" exit 1; + + - name: "✅ I should run, expr: €{{ env.SOMETHING_FALSE == 'true' }} && true" + id: step56 + if: ${{ env.SOMETHING_FALSE == 'true' }} && true + run: echo OK + + - name: "Double checking expr: €{{ env.SOMETHING_FALSE == 'true' }} && true" + if: steps.step56.conclusion == 'skipped' + run: echo "${{ env.SOMETHING_FALSE == 'true' }} && true should have been true, but wasn't" + + - name: "✅ I should run, expr: true && €{{ env.SOMETHING_FALSE == 'true' }}" + id: step57 + if: true && ${{ env.SOMETHING_FALSE == 'true' }} + run: echo OK + + - name: "Double checking expr: true && €{{ env.SOMETHING_FALSE == 'true' }}" + if: steps.step57.conclusion == 'skipped' + run: echo "true && ${{ env.SOMETHING_FALSE == 'true' }} should have been true, but wasn't" diff --git a/pkg/runner/expression.go b/pkg/runner/expression.go index bf19265..77e161b 100644 --- a/pkg/runner/expression.go +++ b/pkg/runner/expression.go @@ -16,11 +16,12 @@ import ( "gopkg.in/godo.v2/glob" ) -var contextPattern, expressionPattern *regexp.Regexp +var contextPattern, expressionPattern, operatorPattern *regexp.Regexp func init() { - contextPattern = regexp.MustCompile(`^(\w+(?:\[.+])*)(?:\.([\w-]+))?(.*)$`) + contextPattern = regexp.MustCompile(`^([^.]*(?:\[.+])*)(?:\.([\w-]+))?(.*)$`) expressionPattern = regexp.MustCompile(`\${{\s*(.+?)\s*}}`) + operatorPattern = regexp.MustCompile("^[!=><|&]+$") } // NewExpressionEvaluator creates a new evaluator @@ -49,8 +50,9 @@ func (sc *StepContext) NewExpressionEvaluator() ExpressionEvaluator { // ExpressionEvaluator is the interface for evaluating expressions type ExpressionEvaluator interface { - Evaluate(string) (string, error) + Evaluate(string) (string, bool, error) Interpolate(string) string + InterpolateWithStringCheck(string) (string, bool) Rewrite(string) string } @@ -58,7 +60,7 @@ type expressionEvaluator struct { vm *otto.Otto } -func (ee *expressionEvaluator) Evaluate(in string) (string, error) { +func (ee *expressionEvaluator) Evaluate(in string) (string, bool, error) { re := ee.Rewrite(in) if re != in { log.Debugf("Evaluating '%s' instead of '%s'", re, in) @@ -66,32 +68,40 @@ func (ee *expressionEvaluator) Evaluate(in string) (string, error) { val, err := ee.vm.Run(re) if err != nil { - return "", err + return "", false, err } if val.IsNull() || val.IsUndefined() { - return "", nil + return "", false, nil } - return val.ToString() + valAsString, err := val.ToString() + if err != nil { + return "", false, err + } + + return valAsString, val.IsString(), err } -func (ee *expressionEvaluator) Interpolate(in string) string { +func (ee *expressionEvaluator) Interpolate(in string) string{ + interpolated, _ := ee.InterpolateWithStringCheck(in) + return interpolated +} + +func (ee *expressionEvaluator) InterpolateWithStringCheck(in string) (string, bool) { errList := make([]error, 0) out := in + isString := false for { out = expressionPattern.ReplaceAllStringFunc(in, func(match string) string { // Extract and trim the actual expression inside ${{...}} delimiters expression := expressionPattern.ReplaceAllString(match, "$1") // Evaluate the expression and retrieve errors if any - negatedExpression := strings.HasPrefix(expression, "!") - evaluated, err := ee.Evaluate(strings.ReplaceAll(expression, "!", "")) + evaluated, evaluatedIsString, err := ee.Evaluate(expression) if err != nil { errList = append(errList, err) } - if negatedExpression { - evaluated = fmt.Sprintf("!%s", evaluated) - } + isString = evaluatedIsString return evaluated }) if len(errList) > 0 { @@ -104,7 +114,7 @@ func (ee *expressionEvaluator) Interpolate(in string) string { } in = out } - return out + return out, isString } // Rewrite tries to transform any javascript property accessor into its bracket notation. diff --git a/pkg/runner/expression_test.go b/pkg/runner/expression_test.go index 5f841ca..cd336e4 100644 --- a/pkg/runner/expression_test.go +++ b/pkg/runner/expression_test.go @@ -1,6 +1,9 @@ package runner import ( + "fmt" + "os" + "regexp" "testing" "github.com/nektos/act/pkg/model" @@ -105,7 +108,7 @@ func TestEvaluate(t *testing.T) { table := table t.Run(table.in, func(t *testing.T) { assert := a.New(t) - out, err := ee.Evaluate(table.in) + out, _, err := ee.Evaluate(table.in) if table.errMesg == "" { assert.NoError(err, table.in) assert.Equal(table.out, out, table.in) @@ -126,8 +129,8 @@ func TestInterpolate(t *testing.T) { "KEYWITHNOTHING": "valuewithnothing", "KEY-WITH-HYPHENS": "value-with-hyphens", "KEY_WITH_UNDERSCORES": "value_with_underscores", - "TRUE": "true", - "FALSE": "false", + "SOMETHING_TRUE": "true", + "SOMETHING_FALSE": "false", }, Run: &model.Run{ JobID: "job1", @@ -149,12 +152,26 @@ func TestInterpolate(t *testing.T) { {" ${{ env.KEY-WITH-HYPHENS }} ", " value-with-hyphens "}, {" ${{ env.KEY_WITH_UNDERSCORES }} ", " value_with_underscores "}, {"${{ env.UNKNOWN }}", ""}, - {"${{ env.TRUE }}", "true"}, - {"${{ env.FALSE }}", "false"}, - {"${{ !env.TRUE }}", "!true"}, - {"${{ !env.FALSE }}", "!false"}, + {"${{ env.SOMETHING_TRUE }}", "true"}, + {"${{ env.SOMETHING_FALSE }}", "false"}, + {"${{ !env.SOMETHING_TRUE }}", "false"}, + {"${{ !env.SOMETHING_FALSE }}", "false"}, + {"${{ !env.SOMETHING_TRUE && true }}", "false"}, + {"${{ !env.SOMETHING_FALSE && true }}", "false"}, + {"${{ env.SOMETHING_TRUE && true }}", "true"}, + {"${{ env.SOMETHING_FALSE && true }}", "true"}, + {"${{ !env.SOMETHING_TRUE || true }}", "true"}, + {"${{ !env.SOMETHING_FALSE || true }}", "true"}, + {"${{ !env.SOMETHING_TRUE && false }}", "false"}, + {"${{ !env.SOMETHING_FALSE && false }}", "false"}, + {"${{ !env.SOMETHING_TRUE || false }}", "false"}, + {"${{ !env.SOMETHING_FALSE || false }}", "false"}, + {"${{ env.SOMETHING_TRUE || false }}", "true"}, + {"${{ env.SOMETHING_FALSE || false }}", "false"}, + {"${{ env.SOMETHING_FALSE }} && ${{ env.SOMETHING_TRUE }}", "false && true"}, } + updateTestExpressionWorkflow(t, tables, rc) for _, table := range tables { table := table t.Run(table.in, func(t *testing.T) { @@ -165,6 +182,56 @@ func TestInterpolate(t *testing.T) { } } +func updateTestExpressionWorkflow(t *testing.T, tables []struct { + in string + out string + //wantErr bool +}, rc *RunContext) { + + var envs string + for k, v := range rc.Env { + envs += fmt.Sprintf( + ` %s: %s +`, k, v) + } + workflow := fmt.Sprintf(` +name: "Test how expressions are handled on Github" +on: push + +env: +%s + +jobs: + test-espressions: + runs-on: ubuntu-latest + steps: +`, envs) + for _, table := range tables { + expressionPattern = regexp.MustCompile(`\${{\s*(.+?)\s*}}`) + + expr := expressionPattern.ReplaceAllStringFunc(table.in, func(match string) string { + return fmt.Sprintf("€{{ %s }}", expressionPattern.ReplaceAllString(match, "$1")) + }) + name := fmt.Sprintf(`%s -> %s should be equal to %s`,expr, table.in, table.out) + echo := `run: echo "Done "` + workflow += fmt.Sprintf(` + - name: %s + %s +`, name, echo) + } + + file, err := os.Create("../../.github/workflows/test-expressions.yml") + if err != nil { + t.Fatal(err) + } + + _, err = file.WriteString(workflow) + if err != nil { + t.Fatal(err) + } + +} + func TestRewrite(t *testing.T) { rc := &RunContext{ Config: &Config{}, diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index 0da5228..7a59a07 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -3,6 +3,7 @@ package runner import ( "context" "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -199,13 +200,19 @@ func (rc *RunContext) newStepExecutor(step *model.Step) common.Executor { _ = sc.setupEnv()(ctx) rc.ExprEval = sc.NewExpressionEvaluator() - if !rc.EvalBool(sc.Step.If) { + runStep, err := rc.EvalBool(sc.Step.If) + if err != nil { + common.Logger(ctx).Errorf(" \u274C Error in if: expression - %s", sc.Step) + rc.StepResults[rc.CurrentStep].Success = false + return err + } + if !runStep { log.Debugf("Skipping step '%s' due to '%s'", sc.Step.String(), sc.Step.If) return nil } common.Logger(ctx).Infof("\u2B50 Run %s", sc.Step) - err := sc.Executor()(ctx) + err = sc.Executor()(ctx) if err == nil { common.Logger(ctx).Infof(" \u2705 Success - %s", sc.Step) } else { @@ -238,7 +245,12 @@ func (rc *RunContext) platformImage() string { func (rc *RunContext) isEnabled(ctx context.Context) bool { job := rc.Run.Job() l := common.Logger(ctx) - if !rc.EvalBool(job.If) { + runJob, err := rc.EvalBool(job.If) + if err != nil { + common.Logger(ctx).Errorf(" \u274C Error in if: expression - %s", job.Name) + return false + } + if !runJob { l.Debugf("Skipping job '%s' due to '%s'", job.Name, job.If) return false } @@ -252,64 +264,59 @@ func (rc *RunContext) isEnabled(ctx context.Context) bool { } // EvalBool evaluates an expression against current run context -func (rc *RunContext) EvalBool(expr string) bool { +func (rc *RunContext) EvalBool(expr string) (bool, error) { + if strings.HasPrefix(strings.TrimSpace(expr), "!") { + return false, errors.New("expressions starting with ! must be wrapped in ${{ }}") + } if expr != "" { - interpolated := rc.ExprEval.Interpolate(expr) - parts := strings.Split(interpolated, " ") + splitPattern := regexp.MustCompile(fmt.Sprintf(`%s|%s|\S+`, expressionPattern.String(), operatorPattern.String())) - operatorRe := regexp.MustCompile("^[!=><|&]+$") + parts := splitPattern.FindAllString(expr, -1) var evaluatedParts []string - for _, part := range parts { - part = fixNegation(part) - - if operatorRe.MatchString(part) { + for i, part := range parts { + if operatorPattern.MatchString(part) { evaluatedParts = append(evaluatedParts, part) continue } - if strings.HasPrefix(part, "!") { - withoutNegation, err := rc.ExprEval.Evaluate(strings.ReplaceAll(part, "!", "")) - if err != nil { - return false - } - evaluatedParts = append(evaluatedParts, fmt.Sprintf("!%s", withoutNegation)) - continue + interpolatedPart, isString := rc.ExprEval.InterpolateWithStringCheck(part) + + // This peculiar transformation has to be done because the Github parser + // treats false retured from contexts as a string, not a boolean. + // Hence env.SOMETHING will be evaluated to true in an if: expression + // regardless if SOMETHING is set to false, true or any other string. + // It also handles some other weirdness that I found by trial and error. + if (expressionPattern.MatchString(part) && // it is an expression + !strings.Contains(part, "!")) && // but it's not negated + interpolatedPart == "false" && // and the interpolated string is false + (isString || previousOrNextPartIsAnOperator(i, parts)) { // and it's of type string or has an logical operator before or after + + interpolatedPart = fmt.Sprintf("'%s'", interpolatedPart) // then we have to quote the false expression } - // strings with / are misinterpreted as a file path by otto - if strings.Contains(part, "/") { - evaluatedParts = append(evaluatedParts, part) - continue - } - evaluatedPart, err := rc.ExprEval.Evaluate(part) - if err != nil { - log.Errorf("Unable to evaluate part: %s: %v", part, err) - return false - } - evaluatedPart = fixQuotingForStrings(evaluatedPart) - evaluatedParts = append(evaluatedParts, evaluatedPart) + + evaluatedParts = append(evaluatedParts, interpolatedPart) } - boolExpr := fmt.Sprintf("Boolean(%s)", strings.Join(evaluatedParts, " ")) - v, err := rc.ExprEval.Evaluate(boolExpr) + joined := strings.Join(evaluatedParts, " ") + v, _, err := rc.ExprEval.Evaluate(fmt.Sprintf("Boolean(%s)", joined)) if err != nil { - return false + return false, nil } log.Debugf("expression '%s' evaluated to '%s'", expr, v) - return v == "true" + return v == "true", nil } - return true + return true, nil } -func fixNegation(s string) string { - re := regexp.MustCompile("![ ]+") - return re.ReplaceAllString(s, "!") -} - -func fixQuotingForStrings(s string) string { - if s == "true" || s == "false" { - return s +func previousOrNextPartIsAnOperator(i int, parts []string) bool { + operator := false + if i > 0 { + operator = operatorPattern.MatchString(parts[i-1]) } - return fmt.Sprintf("'%s'", s) + if i+1 < len(parts) { + operator = operator || operatorPattern.MatchString(parts[i+1]) + } + return operator } func mergeMaps(maps ...map[string]string) map[string]string { diff --git a/pkg/runner/run_context_test.go b/pkg/runner/run_context_test.go index 9b9c945..07b27dd 100644 --- a/pkg/runner/run_context_test.go +++ b/pkg/runner/run_context_test.go @@ -4,6 +4,9 @@ import ( "fmt" "github.com/nektos/act/pkg/model" a "github.com/stretchr/testify/assert" + "os" + "regexp" + "strings" "testing" "github.com/sirupsen/logrus/hooks/test" @@ -16,9 +19,9 @@ func TestRunContext_EvalBool(t *testing.T) { Workdir: ".", }, Env: map[string]string{ - "TRUE": "true", - "FALSE": "false", - "SOME_TEXT": "text", + "SOMETHING_TRUE": "true", + "SOMETHING_FALSE": "false", + "SOME_TEXT": "text", }, Run: &model.Run{ JobID: "job1", @@ -52,73 +55,158 @@ func TestRunContext_EvalBool(t *testing.T) { rc.ExprEval = rc.NewExpressionEvaluator() tables := []struct { - in string - out bool + in string + out bool + wantErr bool }{ // The basic ones - {"true", true}, - {"false", false}, - {"1 != 0", true}, - {"1 != 1", false}, - {"1 == 0", false}, - {"1 == 1", true}, - {"1 > 2", false}, - {"1 < 2", true}, - {"success()", true}, - {"failure()", false}, - {"always()", true}, - {"failure()", false}, + {in: "failure()", out: false}, + {in: "success()", out: true}, + {in: "cancelled()", out: false}, + {in: "always()", out: true}, + {in: "true", out: true}, + {in: "false", out: false}, + {in: "!true", wantErr: true}, + {in: "!false", wantErr: true}, + {in: "1 != 0", out: true}, + {in: "1 != 1", out: false}, + {in: "${{ 1 != 0 }}", out: true}, + {in: "${{ 1 != 1 }}", out: false}, + {in: "1 == 0", out: false}, + {in: "1 == 1", out: true}, + {in: "1 > 2", out: false}, + {in: "1 < 2", out: true}, // And or - {"true && false", false}, - {"true && 1 < 2", true}, - {"false || 1 < 2", true}, - {"false || false", false}, + {in: "true && false", out: false}, + {in: "true && 1 < 2", out: true}, + {in: "false || 1 < 2", out: true}, + {in: "false || false", out: false}, // None boolable - {"env.UNKNOWN == 'true'", false}, - {"env.UNKNOWN", false}, + {in: "env.UNKNOWN == 'true'", out: false}, + {in: "env.UNKNOWN", out: false}, // Inline expressions - {"env.SOME_TEXT", true}, // this is because Boolean('text') is true in Javascript - {"env.SOME_TEXT == 'text'", true}, - {"env.TRUE == 'true'", true}, - {"env.FALSE == 'true'", false}, - {"env.TRUE", true}, - {"env.FALSE", false}, - {"!env.TRUE", false}, - {"!env.FALSE", true}, - {"${{ env.TRUE }}", true}, - {"${{ env.FALSE }}", false}, - {"${{ !env.TRUE }}", false}, - {"${{ !env.FALSE }}", true}, - {"!env.TRUE && true", false}, - {"!env.FALSE && true", true}, - {"!env.TRUE || true", true}, - {"!env.FALSE || false", true}, - {"${{env.TRUE == 'true'}}", true}, - {"${{env.FALSE == 'true'}}", false}, - {"${{env.FALSE == 'false'}}", true}, + {in: "env.SOME_TEXT", out: true}, // this is because Boolean('text') is true in Javascript + {in: "env.SOME_TEXT == 'text'", out: true}, + {in: "env.SOMETHING_TRUE == 'true'", out: true}, + {in: "env.SOMETHING_FALSE == 'true'", out: false}, + {in: "env.SOMETHING_TRUE", out: true}, + {in: "env.SOMETHING_FALSE", out: true}, // this is because Boolean('text') is true in Javascript + {in: "!env.SOMETHING_TRUE", wantErr: true}, + {in: "!env.SOMETHING_FALSE", wantErr: true}, + {in: "${{ !env.SOMETHING_TRUE }}", out: false}, + {in: "${{ !env.SOMETHING_FALSE }}", out: false}, + {in: "${{ ! env.SOMETHING_TRUE }}", out: false}, + {in: "${{ ! env.SOMETHING_FALSE }}", out: false}, + {in: "${{ env.SOMETHING_TRUE }}", out: true}, + {in: "${{ env.SOMETHING_FALSE }}", out: true}, + {in: "${{ !env.SOMETHING_TRUE }}", out: false}, + {in: "${{ !env.SOMETHING_FALSE }}", out: false}, + {in: "${{ !env.SOMETHING_TRUE && true }}", out: false}, + {in: "${{ !env.SOMETHING_FALSE && true }}", out: false}, + {in: "${{ !env.SOMETHING_TRUE || true }}", out: true}, + {in: "${{ !env.SOMETHING_FALSE || false }}", out: false}, + {in: "${{ env.SOMETHING_TRUE && true }}", out: true}, + {in: "${{ env.SOMETHING_FALSE || true }}", out: true}, + {in: "${{ env.SOMETHING_FALSE || false }}", out: true}, + {in: "!env.SOMETHING_TRUE || true", wantErr: true}, + {in: "${{ env.SOMETHING_TRUE == 'true'}}", out: true}, + {in: "${{ env.SOMETHING_FALSE == 'true'}}", out: false}, + {in: "${{ env.SOMETHING_FALSE == 'false'}}", out: true}, + {in: "${{ env.SOMETHING_FALSE }} && ${{ env.SOMETHING_TRUE }}", out: true}, // All together now - {"false || env.TRUE == 'true'", true}, - {"true || env.FALSE == 'true'", true}, - {"true && env.TRUE == 'true'", true}, - {"false && env.TRUE == 'true'", false}, - {"env.FALSE == 'true' && env.TRUE == 'true'", false}, - {"env.FALSE == 'true' && true", false}, - {"${{env.FALSE == 'true'}} && true", false}, + {in: "false || env.SOMETHING_TRUE == 'true'", out: true}, + {in: "true || env.SOMETHING_FALSE == 'true'", out: true}, + {in: "true && env.SOMETHING_TRUE == 'true'", out: true}, + {in: "false && env.SOMETHING_TRUE == 'true'", out: false}, + {in: "env.SOMETHING_FALSE == 'true' && env.SOMETHING_TRUE == 'true'", out: false}, + {in: "env.SOMETHING_FALSE == 'true' && true", out: false}, + {in: "${{ env.SOMETHING_FALSE == 'true' }} && true", out: true}, + {in: "true && ${{ env.SOMETHING_FALSE == 'true' }}", out: true}, // Check github context - {"github.actor == 'nektos/act'", true}, - {"github.actor == 'unknown'", false}, + {in: "github.actor == 'nektos/act'", out: true}, + {in: "github.actor == 'unknown'", out: false}, } + updateTestIfWorkflow(t, tables, rc) for _, table := range tables { table := table t.Run(table.in, func(t *testing.T) { assert := a.New(t) defer hook.Reset() - b := rc.EvalBool(table.in) + b, err := rc.EvalBool(table.in) + if table.wantErr { + assert.Error(err) + } assert.Equal(table.out, b, fmt.Sprintf("Expected %s to be %v, was %v", table.in, table.out, b)) assert.Empty(hook.LastEntry(), table.in) }) } } + +func updateTestIfWorkflow(t *testing.T, tables []struct { + in string + out bool + wantErr bool +}, rc *RunContext) { + + var envs string + for k, v := range rc.Env { + envs += fmt.Sprintf( + ` %s: %s +`, k, v) + } + workflow := fmt.Sprintf(` +name: "Test what expressions result in true and false on Github" +on: push + +env: +%s + +jobs: + test-ifs-and-buts: + runs-on: ubuntu-latest + steps: +`, envs) + + for i, table := range tables { + if table.wantErr || strings.HasPrefix(table.in, "github.actor") { + continue + } + expressionPattern = regexp.MustCompile(`\${{\s*(.+?)\s*}}`) + + expr := expressionPattern.ReplaceAllStringFunc(table.in, func(match string) string { + return fmt.Sprintf("€{{ %s }}", expressionPattern.ReplaceAllString(match, "$1")) + }) + echo := fmt.Sprintf(`run: echo "%s should be false, but was evaluated to true;" exit 1;`, table.in) + name := fmt.Sprintf(`"❌ I should not run, expr: %s"`, expr) + if table.out { + echo = `run: echo OK` + name = fmt.Sprintf(`"✅ I should run, expr: %s"`, expr) + } + workflow += fmt.Sprintf(` + - name: %s + id: step%d + if: %s + %s +`, name, i, table.in, echo) + if table.out { + workflow += fmt.Sprintf(` + - name: "Double checking expr: %s" + if: steps.step%d.conclusion == 'skipped' + run: echo "%s should have been true, but wasn't" +`, expr, i, table.in) + } + } + + file, err := os.Create("../../.github/workflows/test-if.yml") + if err != nil { + t.Fatal(err) + } + + _, err = file.WriteString(workflow) + if err != nil { + t.Fatal(err) + } +}