Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EvalBool and Interpolation fixes #424

Merged
merged 3 commits into from
Nov 17, 2020
Merged

Conversation

torbjornvatn
Copy link
Contributor

@torbjornvatn torbjornvatn commented Nov 16, 2020

I had take another stab at fixing the Interpolation and EvalBool functions so their results match the somewhat weird ones performed by live Github actions.

I've added some auto generated workflows that verifies that the assumptions we make about how Github evaluates stuff is actually true. So the EvalBool test in run_context_test.go no checks that act evaluates expression and booleans the same as Github does.
Screenshot 2020-11-16 at 23 36 46

I had to make some really weird exceptions to make it work though, to me it seems that the expression parser that the Github actions runner either has some bugs or the developers has made some strange design choices.
I've tried to explain the weirdness in the comments here: https://github.com/unacast/act/blob/eval_and_interpolation/pkg/runner/run_context.go#L289

The test-expressions.yml and test-if.yml workflow files are auto generated every time the expression and run_context tests are run, to make sure that future changes to the tests (because the code has changed) or to the Github actions runner are detected.

Some of the issues that this PR tries to solve are described in #353 and #366

Fixes #353
Fixes #366

@cplee
Copy link
Contributor

cplee commented Nov 16, 2020

Hey @torbjornvatn - thanks for all the work here!

Question for you though. This looks to have some overlap with #417. Which should I be looking at first?

@torbjornvatn
Copy link
Contributor Author

Hey @torbjornvatn - thanks for all the work here!

Question for you though. This looks to have some overlap with #417. Which should I be looking at first?

No, there's no overlap. It's just that I merged this branch into the #417 branch.
But if that makes it hard for you to follow, I can just close #417 for now and make a new PR from master when/if this PR gets merged.

This PR is the most important to look at since it's fixing some issues (hopefully)

Copy link
Contributor

@cplee cplee left a comment

Choose a reason for hiding this comment

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

This is great, thanks @torbjornvatn !!

@cplee cplee merged commit 8ba3306 into nektos:master Nov 17, 2020
@cplee cplee mentioned this pull request Nov 17, 2020
@torbjornvatn torbjornvatn deleted the eval_and_interpolation branch November 18, 2020 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants