Skip to content
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

feat: add testing helpers and instructions #392

Merged
merged 1 commit into from Dec 1, 2021
Merged

Conversation

matthewrobertson
Copy link
Member

@matthewrobertson matthewrobertson commented Nov 20, 2021

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

Copy link
Contributor

@anniefu anniefu left a 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 😃

docs/testing-functions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@grant grant left a 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.

docs/testing-functions.md Outdated Show resolved Hide resolved
docs/testing-functions.md Outdated Show resolved Hide resolved
docs/testing-functions.md Outdated Show resolved Hide resolved
docs/testing-functions.md Outdated Show resolved Hide resolved
docs/testing-functions.md Outdated Show resolved Hide resolved
test/integration/cloud_event.ts Outdated Show resolved Hide resolved
test/integration/legacy_event.ts Outdated Show resolved Hide resolved
src/testing.ts Outdated
*
* @beta
*/
export const testCloudEvent = (
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

docs/testing-functions.md Show resolved Hide resolved
docs/testing-functions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@grant grant left a 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.

docs/testing-functions.md Outdated Show resolved Hide resolved
docs/testing-functions.md Show resolved Hide resolved
docs/testing-functions.md Outdated Show resolved Hide resolved
docs/testing-functions.md Outdated Show resolved Hide resolved
src/testing.ts Outdated
*
* @beta
*/
export const testCloudEvent = (
Copy link
Contributor

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.

docs/testing-functions.md Outdated Show resolved Hide resolved
Comment on lines +37 to +33
/**
* 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;
};
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

src/testing.ts Outdated Show resolved Hide resolved
src/testing.ts Outdated Show resolved Hide resolved
test/integration/cloud_event.ts Outdated Show resolved Hide resolved
@grant
Copy link
Contributor

grant commented Dec 1, 2021

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:

  • Adds a @google-cloud/functions-framework/testing package
    • Includes X, Y, Z interfaces
  • This testing utility is one-off for the Node FF

Docs:

  • Adds a guide on testing functions
    • sample for loading, registering, and running tests using assert
    • testing HTTP functions
    • testing CloudEvent functions

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.

@matthewrobertson
Copy link
Member Author

We typically defer heavier documentation to DevSite docs as there can be a mix of guidance.

@grant currently none of our Node.js docs on DevSite are using the declarative function signature APIs, so this guide and the testing module are incompatible with those docs / samples. Once we update all (most?) of the other docs on DevSet to use declarative I think we should update this guide to align it with the docs and helpers I am adding here. The plan for that is still TBD so I think checking some docs in here is a good interim solution.

Also FWIW the dotnet and ruby repos already include similar markdown docs that cover testing functions.

Copy link
Contributor

@grant grant left a 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.

test/integration/cloud_event.ts Outdated Show resolved Hide resolved
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document the unit testing story
3 participants