envs in if: - take 2 (#412)

* Test more if env variants

* The correct negation syntax is !=

* Make the Interpolate function support negated booleans from envs

* Move assert := a.New(t) into t.Run

This uncovered that some of the test premisses was wrong and the
Eval Bool function also had flaws

* Remove a stray logrus import

go.mod

@@ -30,7 +30,7 @@ require (
 	github.com/morikuni/aec v1.0.0 // indirect
 	github.com/opencontainers/go-digest v1.0.0-rc1 // indirect
 	github.com/opencontainers/image-spec v1.0.1 // indirect
-	github.com/opencontainers/runc v0.1.1 // indirect
+	github.com/opencontainers/runc v0.1.1
 	github.com/pkg/errors v0.8.1
 	github.com/robertkrimen/otto v0.0.0-20191219234010-c382bd3c16ff
 	github.com/sabhiram/go-gitignore v0.0.0-20180611051255-d3107576ba94

pkg/runner/expression.go

@@ -12,7 +12,6 @@ import (
 	"strings"
 
 	"github.com/robertkrimen/otto"
-	"github.com/sirupsen/logrus"
 	log "github.com/sirupsen/logrus"
 	"gopkg.in/godo.v2/glob"
 )
@@ -20,7 +19,7 @@ import (
 var contextPattern, expressionPattern *regexp.Regexp
 
 func init() {
-	contextPattern = regexp.MustCompile(`^(\w+(?:\[.+\])*)(?:\.([\w-]+))?(.*)$`)
+	contextPattern = regexp.MustCompile(`^(\w+(?:\[.+])*)(?:\.([\w-]+))?(.*)$`)
 	expressionPattern = regexp.MustCompile(`\${{\s*(.+?)\s*}}`)
 }
 
@@ -62,7 +61,7 @@ type expressionEvaluator struct {
 func (ee *expressionEvaluator) Evaluate(in string) (string, error) {
 	re := ee.Rewrite(in)
 	if re != in {
-		logrus.Debugf("Evaluating '%s' instead of '%s'", re, in)
+		log.Debugf("Evaluating '%s' instead of '%s'", re, in)
 	}
 
 	val, err := ee.vm.Run(re)
@@ -83,15 +82,20 @@ func (ee *expressionEvaluator) Interpolate(in string) string {
 		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
-			evaluated, err := ee.Evaluate(expression)
+			negatedExpression := strings.HasPrefix(expression, "!")
+			evaluated, err := ee.Evaluate(strings.ReplaceAll(expression, "!", ""))
 			if err != nil {
 				errList = append(errList, err)
 			}
+			if negatedExpression {
+				evaluated = fmt.Sprintf("!%s", evaluated)
+			}
 			return evaluated
 		})
 		if len(errList) > 0 {
-			logrus.Errorf("Unable to interpolate string '%s' - %v", in, errList)
+			log.Errorf("Unable to interpolate string '%s' - %v", in, errList)
 			break
 		}
 		if out == in {
@@ -211,7 +215,7 @@ func vmToJSON(vm *otto.Otto) {
 	toJSON := func(o interface{}) string {
 		rtn, err := json.MarshalIndent(o, "", "  ")
 		if err != nil {
-			logrus.Errorf("Unable to marshal: %v", err)
+			log.Errorf("Unable to marshal: %v", err)
 			return ""
 		}
 		return string(rtn)
@@ -225,7 +229,7 @@ func vmFromJSON(vm *otto.Otto) {
 		var dat map[string]interface{}
 		err := json.Unmarshal([]byte(str), &dat)
 		if err != nil {
-			logrus.Errorf("Unable to unmarshal: %v", err)
+			log.Errorf("Unable to unmarshal: %v", err)
 			return dat
 		}
 		return dat
@@ -237,11 +241,11 @@ func vmFromJSON(vm *otto.Otto) {
 func (rc *RunContext) vmHashFiles() func(*otto.Otto) {
 	return func(vm *otto.Otto) {
 		_ = vm.Set("hashFiles", func(paths ...string) string {
-			files := []*glob.FileAsset{}
+			var files []*glob.FileAsset
 			for i := range paths {
 				newFiles, _, err := glob.Glob([]string{filepath.Join(rc.Config.Workdir, paths[i])})
 				if err != nil {
-					logrus.Errorf("Unable to glob.Glob: %v", err)
+					log.Errorf("Unable to glob.Glob: %v", err)
 					return ""
 				}
 				files = append(files, newFiles...)
@@ -250,11 +254,13 @@ func (rc *RunContext) vmHashFiles() func(*otto.Otto) {
 			for _, file := range files {
 				f, err := os.Open(file.Path)
 				if err != nil {
-					logrus.Errorf("Unable to os.Open: %v", err)
+					log.Errorf("Unable to os.Open: %v", err)
 				}
-				defer f.Close()
 				if _, err := io.Copy(hasher, f); err != nil {
-					logrus.Errorf("Unable to io.Copy: %v", err)
+					log.Errorf("Unable to io.Copy: %v", err)
+				}
+				if err := f.Close(); err != nil {
+					log.Errorf("Unable to Close file: %v", err)
 				}
 			}
 			return hex.EncodeToString(hasher.Sum(nil))

pkg/runner/expression_test.go

@@ -8,7 +8,6 @@ import (
 )
 
 func TestEvaluate(t *testing.T) {
-	assert := a.New(t)
 	rc := &RunContext{
 		Config: &Config{
 			Workdir: ".",
@@ -105,6 +104,7 @@ func TestEvaluate(t *testing.T) {
 	for _, table := range tables {
 		table := table
 		t.Run(table.in, func(t *testing.T) {
+			assert := a.New(t)
 			out, err := ee.Evaluate(table.in)
 			if table.errMesg == "" {
 				assert.NoError(err, table.in)
@@ -118,15 +118,16 @@ func TestEvaluate(t *testing.T) {
 }
 
 func TestInterpolate(t *testing.T) {
-	assert := a.New(t)
 	rc := &RunContext{
 		Config: &Config{
 			Workdir: ".",
 		},
 		Env: map[string]string{
-			"keywithnothing":       "valuewithnothing",
+			"KEYWITHNOTHING":       "valuewithnothing",
-			"key-with-hyphens":     "value-with-hyphens",
+			"KEY-WITH-HYPHENS":     "value-with-hyphens",
-			"key_with_underscores": "value_with_underscores",
+			"KEY_WITH_UNDERSCORES": "value_with_underscores",
+			"TRUE":                 "true",
+			"FALSE":                "false",
 		},
 		Run: &model.Run{
 			JobID: "job1",
@@ -144,15 +145,20 @@ func TestInterpolate(t *testing.T) {
 		out string
 	}{
 		{" ${{1}} to ${{2}} ", " 1 to 2 "},
-		{" ${{ env.keywithnothing }} ", " valuewithnothing "},
+		{" ${{ env.KEYWITHNOTHING }} ", " valuewithnothing "},
-		{" ${{ env.key-with-hyphens }} ", " value-with-hyphens "},
+		{" ${{ env.KEY-WITH-HYPHENS }} ", " value-with-hyphens "},
-		{" ${{ env.key_with_underscores }} ", " value_with_underscores "},
+		{" ${{ env.KEY_WITH_UNDERSCORES }} ", " value_with_underscores "},
-		{"${{ env.unknown }}", ""},
+		{"${{ env.UNKNOWN }}", ""},
+		{"${{ env.TRUE }}", "true"},
+		{"${{ env.FALSE }}", "false"},
+		{"${{ !env.TRUE }}", "!true"},
+		{"${{ !env.FALSE }}", "!false"},
 	}
 
 	for _, table := range tables {
 		table := table
 		t.Run(table.in, func(t *testing.T) {
+			assert := a.New(t)
 			out := ee.Interpolate(table.in)
 			assert.Equal(table.out, out, table.in)
 		})
@@ -160,8 +166,6 @@ func TestInterpolate(t *testing.T) {
 }
 
 func TestRewrite(t *testing.T) {
-	assert := a.New(t)
-
 	rc := &RunContext{
 		Config: &Config{},
 		Run: &model.Run{
@@ -195,6 +199,7 @@ func TestRewrite(t *testing.T) {
 	for _, table := range tables {
 		table := table
 		t.Run(table.in, func(t *testing.T) {
+			assert := a.New(t)
 			re := ee.Rewrite(table.in)
 			assert.Equal(table.re, re, table.in)
 		})

pkg/runner/run_context.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/nektos/act/pkg/common"
 	"github.com/nektos/act/pkg/model"
-	"github.com/sirupsen/logrus"
 	log "github.com/sirupsen/logrus"
 )
 
@@ -255,8 +254,43 @@ func (rc *RunContext) isEnabled(ctx context.Context) bool {
 // EvalBool evaluates an expression against current run context
 func (rc *RunContext) EvalBool(expr string) bool {
 	if expr != "" {
-		expr = fmt.Sprintf("Boolean(%s)", rc.ExprEval.Interpolate(expr))
+		interpolated := rc.ExprEval.Interpolate(expr)
-		v, err := rc.ExprEval.Evaluate(expr)
+		parts := strings.Split(interpolated, " ")
+
+		operatorRe := regexp.MustCompile("^[!=><|&]+$")
+		var evaluatedParts []string
+		for _, part := range parts {
+			part = fixNegation(part)
+
+			if operatorRe.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
+			}
+			// 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)
+		}
+
+		boolExpr := fmt.Sprintf("Boolean(%s)", strings.Join(evaluatedParts, " "))
+		v, err := rc.ExprEval.Evaluate(boolExpr)
 		if err != nil {
 			return false
 		}
@@ -266,6 +300,18 @@ func (rc *RunContext) EvalBool(expr string) bool {
 	return true
 }
 
+func fixNegation(s string) string {
+	re := regexp.MustCompile("![ ]+")
+	return re.ReplaceAllString(s, "!")
+}
+
+func fixQuotingForStrings(s string) string {
+	if s == "true" || s == "false" {
+		return s
+	}
+	return fmt.Sprintf("'%s'", s)
+}
+
 func mergeMaps(maps ...map[string]string) map[string]string {
 	rtnMap := make(map[string]string)
 	for _, m := range maps {
@@ -403,7 +449,7 @@ func (rc *RunContext) getGithubContext() *githubContext {
 	if rc.EventJSON != "" {
 		err = json.Unmarshal([]byte(rc.EventJSON), &ghc.Event)
 		if err != nil {
-			logrus.Errorf("Unable to Unmarshal event '%s': %v", rc.EventJSON, err)
+			log.Errorf("Unable to Unmarshal event '%s': %v", rc.EventJSON, err)
 		}
 	}
 

pkg/runner/run_context_test.go

@@ -1,6 +1,7 @@
 package runner
 
 import (
+	"fmt"
 	"github.com/nektos/act/pkg/model"
 	a "github.com/stretchr/testify/assert"
 	"testing"
@@ -10,7 +11,6 @@ import (
 
 func TestRunContext_EvalBool(t *testing.T) {
 	hook := test.NewGlobal()
-	assert := a.New(t)
 	rc := &RunContext{
 		Config: &Config{
 			Workdir: ".",
@@ -58,29 +58,45 @@ func TestRunContext_EvalBool(t *testing.T) {
 		// The basic ones
 		{"true", true},
 		{"false", false},
-		{"1 !== 0", true},
+		{"1 != 0", true},
-		{"1 !== 1", false},
+		{"1 != 1", false},
 		{"1 == 0", false},
 		{"1 == 1", true},
 		{"1 > 2", false},
 		{"1 < 2", true},
 		{"success()", true},
 		{"failure()", false},
+		{"always()", true},
+		{"failure()", false},
 		// And or
 		{"true && false", false},
 		{"true && 1 < 2", true},
 		{"false || 1 < 2", true},
 		{"false || false", false},
 		// None boolable
-		{"env.SOME_TEXT", true},
 		{"env.UNKNOWN == 'true'", false},
 		{"env.UNKNOWN", 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},
+
 		// All together now
 		{"false || env.TRUE == 'true'", true},
 		{"true || env.FALSE == 'true'", true},
@@ -97,10 +113,11 @@ func TestRunContext_EvalBool(t *testing.T) {
 	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)
 
-			assert.Equal(table.out, b, table.in)
+			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)
 		})
 	}