-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
chore: add batch execution to CloudDataSource #22457
chore: add batch execution to CloudDataSource #22457
Conversation
Thanks for taking the time to open a PR!
|
1 flaky tests on run #43438 ↗︎
Details:
cypress/e2e/next.cy.ts • 1 flaky test • webpack-dev-server
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
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.
Everything seems to work well. There are tests failing here that don't seem flaky and don't seem to be failing in the feature branch, are you going to address those before merging down?
Yeah I was missing the merge with the last commit |
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.
🤯 Very cool! Works well on my local
@@ -133,6 +143,10 @@ declare global { | |||
* Gives the ability to intercept the remote GraphQL request & respond accordingly | |||
*/ | |||
remoteGraphQLIntercept: typeof remoteGraphQLIntercept | |||
/** | |||
* Gives the ability to intercept the remote GraphQL request & respond accordingly |
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.
Nitpick - "intercept a batched remote Graph request"?
const re = /^_(\d+)_(.*?)$/.exec(key) | ||
|
||
if (!re) { | ||
finalVal[key] = val | ||
continue | ||
} | ||
|
||
const [, capture1, capture2] = re | ||
const subqueryVariables = _.transform(_.pickBy(variables, (val, key) => key.startsWith(`_${capture1}_`)), (acc, val, k) => { | ||
acc[k.replace(`_${capture1}_`, '')] = val | ||
}, {}) |
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.
This section could use a few comments
values.push(Promise.resolve().then(() => { | ||
return fn({ | ||
key, | ||
index: Number(capture1), | ||
field: capture2, | ||
variables: subqueryVariables, | ||
result: result[key], | ||
}, testState) | ||
})) |
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.
Could this be simplified by just resolving the call to fn
? Or is the then
needed to not block processing in this loop?
values.push(Promise.resolve(
fn({
key,
index: Number(capture1),
field: capture2,
variables: subqueryVariables,
result: result[key],
}, testState),
))
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.
The then is to make it so that if fn
is sync an an error is thrown it's it's caught & rejected promise on the field level, rather than being thrown at the top level
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.
Added explicit handling for the errors though so that it does what was intended here
export type RemoteGraphQLInterceptor = (obj: RemoteGraphQLInterceptPayload, testState: Record<string, any>) => ExecutionResult | Promise<ExecutionResult> | Response | ||
|
||
export type RemoteGraphQLBatchInterceptor = (obj: RemoteGraphQLBatchInterceptPayload, testState: Record<string, any>) => any | Promise<any> |
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.
Can we make the return type any more specific? Does the batch return an ExecutionResult
similar to the non-batch signature above?
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.
Yeah, let me make it a generic so we can add it to the signature if we want
2802981
to
2ad75e1
Compare
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 the updates!
const finalVal: Record<string, any> = {} | ||
const errors: GraphQLError[] = [] | ||
|
||
// The batch execution plugin (https://www.graphql-tools.com/docs/batch-execution) batches the |
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.
Nice, this is neat.
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.
I did not pull it down but I grok the general idea. I haven't written an ACI code yet, hoping to do so soon to get my hands dirty in this area of the code base and really internalize it + the dev workflow.
…est-runs-batching
@tgriesser Looks like there may be some legit test failures that need to be fixed so this can merge. |
…est-runs-batching
…ing' of https://github.com/cypress-io/cypress into tgriesser/CLOUD-577-spec-list-display-latest-runs-batching
…est-runs-batching
I moved it to draft - waiting some changes on the cloud side so this can be properly handle |
…est-runs-batching
…est-runs-batching
…est-runs-batching
…est-runs-batching
…est-runs-batching
@estrada9166 Can you link to the cloud side PR or issue we need to track to determine when this is ready? |
…est-runs-batching
…est-runs-batching
…est-runs-batching
…est-runs-batching
…est-runs-batching
* develop: (27 commits) refactor: remove unused cloud routes (#25584) chore: fix issue template formatting issue (#25587) fix: fixed issue with CT + electron + run mode not exiting properly (#25585) chore(deps): update dependency ua-parser-js to v0.7.33 [security] (#25561) fix: add alternative binary names for edge-beta (#25456) chore: add batch execution to CloudDataSource (#22457) chore: End a/b campaigns for aci smart banners (#25504) chore: release @cypress/schematic-v2.5.0 fix(cypress-schematic): do not disable e2e support file (#25400) chore: adding memory issue template (#25559) feat: Add Angular CT Schematic (#24374) chore: enforce changelog entries on PR reviews (#25459) chore: bump package.json to 12.4.0 [run ci] (#25556) feat: Add 'type' option to `.as` to store aliases by value (#25251) chore: release @cypress/webpack-dev-server-v3.2.3 feat: Display line break in cy.log (#25467) chore: update types (#25538) fix: Revert "fix: adding emergency garbage collection for chromium-based browsers" (#25546) fix: percy - wait to take snapshot until previous tooltips are gone (#25522) feat: support data-qa selector in selector playground (#25475) ...
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
Addresses @flotwig comment on #21250 re: network queries, batching multiple GraphQL requests into a single remote request with rewritten variables, using
@graphql-tools/batch-execute
. This makes it simpler to batch on the remote server.The documentation here explains how the merging works: https://www.graphql-tools.com/docs/batch-execution
Additionally only does this in situations where we have multiple requests to batch, so we don't end up with rewritten queries on every request, but just the ones where we have multiple requests. Limits the batching to max 20 items per-query
Steps to test
How has the user experience changed?
Before:
dashboard.-.22.June.2022.mp4
After:
dashboard.-.22.June.2022.1.mp4
PR Tasks
cypress-documentation
?type definitions
?