-
Notifications
You must be signed in to change notification settings - Fork 135
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
breaking(go): update flow streaming protocol to SSE #1316
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
c7b91b2
to
ebb72b8
Compare
tests/src/flow_server_test.ts
Outdated
body: JSON.stringify(test.post), | ||
} as RequestInit; | ||
|
||
await fetchData(`${url}/${test.path}`, fetchopts); |
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 wonder if it would be better to use genkit/client
library here for the test?
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.
Ah, good idea. Could you illuminate me on why using genkit/client
would be better for this? Is it for compatibility reasons?
Using the raw request, we can inspect headers and response payloads to ensure the proper formatting.
|
||
# TODO: add more test cases | ||
|
||
app: flow_server |
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.
you're not testing the streamed data?
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.
Yes. My understanding is that this file should be used for test cases for the flow server, unless this label is for something else that I'm not aware of.
Should I change it to something more specific? e.g. app: stream_flows
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 think Pavel means that you're not consuming the whole stream, just one chunk. Now that I think about it, you do need to consume the stream so you can check the final response.
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.
Got it. I'll add the expected stream values and test against them 😄
6043a56
to
cb99b3c
Compare
OK
ERROR