-
Notifications
You must be signed in to change notification settings - Fork 83
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
test: Expand tests for built-in functions #129
Conversation
4c5122d
to
8d0a2e4
Compare
Thanks for running the GitHub Action, saw it was failing yesterday.
|
@simicd I don't have permissions to officially review but wanted to say changes look good. More test coverage is always a good thing |
Thanks @jdye64! One question: When you want to update a branch after submitting a PR, do you rebase or just merge main into your branch? Asking because it seems like Actions require someone to trigger the workflow again, hope I didn't pick the wrong option by rebasing. |
I always prefer using The workflows should be triggered manually for first time contributors. Once your first PR is merged all following PRs will execute the checks automatically! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @simicd
Thanks for the review @andygrove and for the explanation @martin-g! |
Which issue does this PR close?
Closes #128
Rationale for this change
Improve test coverage for buit-in functions
What changes are included in this PR?
Are there any user-facing changes?
No