fix: coerce booleans to numbers for comparison in exprparser

[ZauberNerd]

No description provided.

[codecov]

Codecov Report

Merging #1030 (a85848e) into master (4f8da0a) will increase coverage by 1.22%.
The diff coverage is 80.25%.

@@            Coverage Diff             @@
##           master    #1030      +/-   ##
==========================================
+ Coverage   57.50%   58.73%   +1.22%     
==========================================
  Files          32       34       +2     
  Lines        4594     4650      +56     
==========================================
+ Hits         2642     2731      +89     
+ Misses       1729     1690      -39     
- Partials      223      229       +6     

Impacted Files	Coverage Δ
pkg/model/action.go	0.00% <ø> (ø)
pkg/model/planner.go	50.73% <ø> (+0.32%)	⬆️
pkg/runner/logger.go	61.76% <64.70%> (-3.67%)	⬇️
pkg/exprparser/interpreter.go	74.39% <66.66%> (+0.99%)	⬆️
pkg/runner/runner.go	74.52% <75.00%> (-1.95%)	⬇️
pkg/runner/expression.go	89.36% <77.61%> (-1.46%)	⬇️
pkg/runner/job_executor.go	81.57% <81.57%> (ø)
pkg/runner/step_context.go	85.81% <84.00%> (+4.17%)	⬆️
pkg/runner/action.go	84.21% <84.21%> (ø)
pkg/runner/run_context.go	80.03% <96.15%> (+0.38%)	⬆️
... and 9 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[cplee]

@ZauberNerd - implementation looks fine, but I'm curious, when is this needed?

[ZauberNerd]

@cplee here is an example workflow that fails:

~/act $ cat test.yml 
on: push
jobs:
  job-1:
    runs-on: ubuntu-latest
    steps:
      - run: echo "hello"
        if: ${{ true == true }}
~/act $ ./dist/local/act -W ./test.yml 
[test.yml/job-1] 🚀  Start image=ghcr.io/catthehacker/ubuntu:act-latest
[test.yml/job-1]   🐳  docker pull image=ghcr.io/catthehacker/ubuntu:act-latest platform= username= forcePull=false
[test.yml/job-1]   🐳  docker create image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[test.yml/job-1]   🐳  docker run image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[test.yml/job-1]   🐳  docker exec cmd=[mkdir -m 0777 -p /var/run/act] user=root workdir=
[test.yml/job-1]   ❌  Error in if-expression: "if: ${{ true == true }}" (TODO: evaluateCompare not implemented reflect.Value)
Error: Job 'job-1' failed

The problem was, that we only coerced to numbers when the inputs are differet:

act/pkg/exprparser/interpreter.go

Lines 298 to 305 in b681c31

	if leftValue.Kind() != rightValue.Kind() {
	if !impl.isNumber(leftValue) {
	leftValue = impl.coerceToNumber(leftValue)
	}
	if !impl.isNumber(rightValue) {
	rightValue = impl.coerceToNumber(rightValue)
	}
	}

but we forgot to implement comparison for booleans:

act/pkg/exprparser/interpreter.go

Lines 307 to 329 in b681c31

	switch leftValue.Kind() {
	case reflect.Bool:
	return impl.compareNumber(float64(impl.coerceToNumber(leftValue).Int()), float64(impl.coerceToNumber(rightValue).Int()), kind)
	case reflect.String:
	return impl.compareString(strings.ToLower(leftValue.String()), strings.ToLower(rightValue.String()), kind)
	case reflect.Int:
	if rightValue.Kind() == reflect.Float64 {
	return impl.compareNumber(float64(leftValue.Int()), rightValue.Float(), kind)
	}
	return impl.compareNumber(float64(leftValue.Int()), float64(rightValue.Int()), kind)
	case reflect.Float64:
	if rightValue.Kind() == reflect.Int {
	return impl.compareNumber(leftValue.Float(), float64(rightValue.Int()), kind)
	}
	return impl.compareNumber(leftValue.Float(), rightValue.Float(), kind)
	default:
	return nil, fmt.Errorf("TODO: evaluateCompare not implemented! left: %+v, right: %+v", leftValue.Kind(), rightValue.Kind())
	}

[cplee]

Ah - == makes sense, I was confused by the > and < test cases. Thanks for the quick fix.

[ZauberNerd]

Ah, yes. I added those tests, to ensure that it doesn't fail for other types of comparison. There is also one test for != which is a bit more useful than > or < ;)

[mergify]

@ZauberNerd this pull request has failed checks 🛠

[ZauberNerd]

codecov upload failed. I'm re-running the workflow now.