-
Notifications
You must be signed in to change notification settings - Fork 275
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
Support unix_ms_now in rhai engine #5181
Support unix_ms_now in rhai engine #5181
Conversation
@shaikatzz: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
CI performance tests
|
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.
Thanks for this PR and for the excellent presentation. I particularly appreciate the test and the docs additions.
Could you also create a changeset (so you get appropriate credit when this is released) following the procedure in .changesets/README.md
? re-request a review from me when you are ready.
Done! |
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.
Test has some potential to be flacky :(
At the same time, I can't suggest a better way to test it.
@IvanGoncharov @garypen do you know why the tests are not passing? what else is required in order to merge that PR? |
possibly because the CLA wasn't signed. I've just kicked it back into life and we'll see what happens after this run. |
I dug into this a little and I think that a change has broken our integration tests so that a PR from a fork will always fail certain integration tests. I'm trying to figure out what has changed so we can fix that and get this merged. |
@garypen I've signed the CLA, let me know if anything else is needed from my side, i'll be happy to have this change merged :-) |
Fixes #5182
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩