-
Notifications
You must be signed in to change notification settings - Fork 167
feat: add testing helpers and instructions #392
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
Conversation
7844b3d
to
499ca87
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.
Neat! I prefer the testing package approach to exposing getters on the FF registration package.
Next up, we generate the test stubs for you 😃
3ae55b2
to
7a0d0b9
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 PR! I've given a review.
Given there's a lot of content: docs, samples, and implementation, we might want to discuss this PR a bit more. Are we going to provide these helpers uniformly across languages? The changes here may also impact other teams.
For some context, the origin of the docs/
folder was to cover topics that aren't covered DevSite and provide a lightweight guide for users using this framework. Some of the frameworks provide guidance within the repo, with guidance also being outside the repo.
src/testing.ts
Outdated
* | ||
* @beta | ||
*/ | ||
export const testCloudEvent = ( |
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 don't know if it makes sense in our Functions Framework API to provide a function to get a test CE object.
If a user relies on this functionality, and we change it accidentally, that would be bad.
We might want to reduce the surface area of our testing API.
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 it is nice for us to provide an API creating stubs of the objects we pass to users functions. This is something we use heavily in our own tests so I suspect developers will get some value out of it as well.
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 really don't think we should provide JSON stubs for events or HTTP requests. That is out of scope for the functions framework contract. I understand your goal to provide a good testing experience for FF developers.
The interface should be kept tight and uniform across the frameworks and should focus on the function -> app logic.
We should be happy to provide this within a sample, but I don't think we should extend our API without a design or discussion.
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.
OK I removed testCloudEvent
and updated the docs to point developers to the JavaScript SDK for CloudEvents for this type of functionality.
ee65782
to
cbe6d7c
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, reviewed! Getting there.
Do you have any thoughts on the comment from before?
Given there's a lot of content: docs, samples, and implementation, we might want to discuss this PR a bit more. Are we going to provide these helpers uniformly across languages? The changes here may also impact other teams.
For some context, the origin of the
docs/
folder was to cover topics that aren't covered DevSite and provide a lightweight guide for users using this framework. Some of the frameworks provide guidance within the repo, with guidance also being outside the repo.
We typically defer heavier documentation to DevSite docs as there can be a mix of guidance.
src/testing.ts
Outdated
* | ||
* @beta | ||
*/ | ||
export const testCloudEvent = ( |
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 really don't think we should provide JSON stubs for events or HTTP requests. That is out of scope for the functions framework contract. I understand your goal to provide a good testing experience for FF developers.
The interface should be kept tight and uniform across the frameworks and should focus on the function -> app logic.
We should be happy to provide this within a sample, but I don't think we should extend our API without a design or discussion.
/** | ||
* Testing utility for retrieving a function registered with the Functions Framework | ||
* @param functionName the name of the function to get | ||
* @returns a function that was registered with the Functions Framework | ||
* | ||
* @beta | ||
*/ | ||
export const getFunction = ( | ||
functionName: string | ||
): HandlerFunction | undefined => { | ||
return getRegisteredFunction(functionName)?.userFunction; | ||
}; |
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.
Discussion: #392 (comment)
I believe it would be reasonable to change the location of the export. Specifically, export getFunction
with the other function registry exports here:
https://github.com/GoogleCloudPlatform/functions-framework-nodejs/blob/master/src/index.ts
That's my suggestion.
I still believe this useful function is not necessarily tied to testing and am suggesting we simply move the export location. There is no testing logic in this function and the same @beta
tag can still be provided there.
We shouldn't mix import locations for testing and registry functions.
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.
My strong preference is to not expose this function from the primary API in index.ts
. My primary concern is that I expect some flux in this API as we add more functionality to the framework (e.g. lifecycle events). Exporting from here sends a strong signal to developers that it's only purpose is for testing and it is not to be used in production.
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 is fine. I don't agree that the location of the export will change developer behavior (see recent internal code location changes for example), but the difference is insignificant.
It would help to have more details as to how this interface is expected to be used here. This utility is useful. This PR is large, substantial, and external-facing. It includes both implementation and documentation. Here's an example PR description that would help: feat: add testing helpers and instructions This PR blocks GoogleCloudPlatform/nodejs-docs-samples#2428 Feat:
Docs:
This content should not go in our docs because of X, Y:
I plan to write this testing guide for Node.js only. We'll follow-up with each language individually. |
cbe6d7c
to
f2ce0b9
Compare
@grant currently none of our Node.js docs on DevSite are using the declarative function signature APIs, so this guide and the Also FWIW the dotnet and ruby repos already include similar markdown docs that cover testing functions. |
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.
Approved. Nothing blocking.
I think we could still use a more detailed PR description. I'm not sure if we will or want to keep docs around this topic. It doesn't seem like we're aiming to do this across FFs.
This PR is urgent, so lgtm.
This commit adds some helpers for unit testing cloud functions along with some documentation about how to use them. I also refactored some of our out tests to use the helpers.
f2ce0b9
to
48d0a96
Compare
This commit adds some helpers for unit testing cloud functions along with some documentation about how to use them. Much of inspiration came from the Ruby Functions Framework: https://github.com/GoogleCloudPlatform/functions-framework-ruby/blob/main/docs/testing-functions.md
A readable preview of the docs is here.
Eventually we will want to update this guide on DevSite to match the guidance in the docs we are adding in this PR; however, at the moment none of our public docs or samples are using the declarative function signature APIs so the helpers / instructions here are not compatible. Once the majority of the nodejs public documentation is updated to use the declarative APIs we can update the testing guide.
I also refactored some of our out tests to use the helpers.
Fixes #387