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

fix: ensure expressions that expect bools return bools #2909

Merged
merged 1 commit into from Feb 28, 2024

Conversation

aliscott
Copy link
Member

@aliscott aliscott commented Feb 28, 2024

If a logical expression (|| or &&) had a sub-expression that was mocked to a non-boolean value, this would always mean the expression always returned false. We now handle this in the same way that we had condition expressions to ensure the mocked expressions return a boolean value if the operator expects boolean values.

@aliscott aliscott self-assigned this Feb 28, 2024
@aliscott aliscott force-pushed the fix-conditional-expr-mock branch 3 times, most recently from 4a00cfe to 4d18f69 Compare February 28, 2024 14:00
@aliscott aliscott marked this pull request as ready for review February 28, 2024 14:15
@aliscott aliscott marked this pull request as draft February 28, 2024 14:16
@aliscott aliscott marked this pull request as ready for review February 28, 2024 14:28
Copy link
Contributor

@hugorut hugorut left a comment

Choose a reason for hiding this comment

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

looks good - minor question about the params size

Comment on lines 601 to 602
lhsParam := params[0]
rhsParam := params[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

is this always expected to be a slice of this size?

Copy link
Contributor

Choose a reason for hiding this comment

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

we should make add a size check just in case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, all BinaryOp functions have 2 params, but updated this anyway to make sure.

@aliscott aliscott merged commit ae1d502 into master Feb 28, 2024
8 of 9 checks passed
@aliscott aliscott deleted the fix-conditional-expr-mock branch February 28, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants