Skip to content

feat: Expose Query Plan to ExecutionRequest (and improve rhai unit test patterns)#2081

Merged
garypen merged 5 commits intodevfrom
garypen/rhai-testing
Nov 15, 2022
Merged

feat: Expose Query Plan to ExecutionRequest (and improve rhai unit test patterns)#2081
garypen merged 5 commits intodevfrom
garypen/rhai-testing

Conversation

@garypen
Copy link
Contributor

@garypen garypen commented Nov 9, 2022

Introduce support for reading the query_plan from an ExecutionRequest

Also, adds unit tests for all rhai request and response accessors.

Now, if a test fails, you'll see something like:

thread 'plugins::rhai::tests::it_can_process_supergraph_request' panicked at 'test failed: ErrorInFunctionCall("process_supergraph_request", "", ErrorRuntime("query: expected: #{\"first\": 1}, actual: #{\"first\": 2}", 60:9), none)', apollo-router/src/plugins/rhai.rs:1749:16

Which is telling you why the rhai script failed (expected != actual) and where it failed: functiona name, line number character offset. That should be enough to figure out what the problem is.

Add unit tests for all rhai `request` and `response` accessors.

If a test fails, you'll see something like:

thread 'plugins::rhai::tests::it_can_process_supergraph_request' panicked at 'test failed: ErrorInFunctionCall("process_supergraph_request", "", ErrorRuntime("query: expected: #{\"first\": 1}, actual: #{\"first\": 2}", 60:9), none)', apollo-router/src/plugins/rhai.rs:1749:16

Which is telling you why the rhai script failed (expected != actual) and
where it failed: functiona name, line number character offset.
That should be enough to figure out what the problem is.

also:
 - add support for reading the query_plan from an ExecutionRequest
 - add contains function for context
@garypen garypen requested review from Geal and bnjjj November 9, 2022 18:09
@garypen garypen self-assigned this Nov 9, 2022
@github-actions

This comment has been minimized.

Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

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

We should try to followup on this with a way to run a test directly from rhai (although i m not sure it s easily feasible ^^'

@garypen garypen merged commit c97c114 into dev Nov 15, 2022
@garypen garypen deleted the garypen/rhai-testing branch November 15, 2022 10:48
@abernix
Copy link
Member

abernix commented Nov 15, 2022

In staging the release, I was confused following the link to this PR from the change log, which refers to it as "Add Query Plan access to ExecutionRequest". I'm going to rename this PR on account of that, but happy to rename it back if that feels wrong.

@abernix abernix changed the title improve our rhai unit testing story feat: Expose Query Plan to ExecutionRequest Nov 15, 2022
@abernix
Copy link
Member

abernix commented Nov 15, 2022

In fact, I've re-worked the whole PR body since I'd claim that adding tests was a thing that was done on the feature, rather than the feature being an accidental outcome of adding Rhai tests? 😄. (Maybe that's how it worked out, I don't know, but it seemed odd to have them intertwined, to me doing the release.)

@abernix abernix changed the title feat: Expose Query Plan to ExecutionRequest feat: Expose Query Plan to ExecutionRequest (and improve rhai testing story) Nov 15, 2022
@abernix abernix changed the title feat: Expose Query Plan to ExecutionRequest (and improve rhai testing story) feat: Expose Query Plan to ExecutionRequest (and improve rhai unit test patterns) Nov 15, 2022
@abernix abernix added this to the v1-NEXT milestone Nov 15, 2022
@garypen
Copy link
Contributor Author

garypen commented Nov 15, 2022

I think your naming is more sensible. I added the improved unit testing stuff and got carried away with it...

@abernix abernix mentioned this pull request Nov 15, 2022
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.

3 participants