Skip to content

Implement firebase-functions-test wrapV2 #131

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

Merged
merged 31 commits into from
May 3, 2022

Conversation

TheIronDev
Copy link
Contributor

Description

Implement wrap function for v2 CloudFuntions.

Code sample

const cloudEventInstance = {..._getDefaultCloudEvent(), data};
const handler = (cloudEvent) => ({cloudEvent});
const cloudFn = storage.onObjectMetadataUpdated('bucket', handler);
expect(cloudFn(cloudEventInstance).cloudEvent).equal(cloudEventInstance);

Verified

This commit was signed with the committer’s verified signature.
arjo129 Arjo Chakravarty
@TheIronDev
Copy link
Contributor Author

Requesting a review from @inlined

This commit makes some minor updates to resolve pr comments.

I still need to go back and clean up the createCloudEvent usecase.

Stay tuned!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This commit implements the createMockCloudEvent.

This simplifies the story of testing the wrapped V2 function.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

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

Unfortunately this SDK is much larger scope to build. The primary reason for this repo isn't because customers can't create mock event envelopes (formerly event contexts). Most of their functions won't even rely on the envelope. The challenge they face is coming up with realistic event data.

For each event type we need a full event where all fields that are generally present have some well known value and, to a best effort, the data is cohesive (e.g. if the customer specifies a bucket, it is the bucket for both the event envelope and the event data).

I have half a mind to suggest that we should actually accept a CloudEvent<T> | T and just add an event wrapper if none of a certain number of fields are actually defined. Or we can just tell everyone that they need to call with {data: whatIActuallyCareAbout} all the time.

});
});

describe('callable functions', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Honestly I have mixed feelings about even supporting wrap for functions that don't have an eventTrigger. We don't do any parsing into native SDK types nor do we fill in any default values. Maybe we should just tell developers to use <theirFn>.run? Thoughts @taeold & @mbleigh?

If we do want to support wrap I wonder if we should be coming up with some sort of helpers to make up an authenticated user, instanceIdToken, and app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was copy/paste of the previous wrap implementation. I don't have strong opinions on this.

Are callable functions documented anywhere for V2?

Copy link

Choose a reason for hiding this comment

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

I can see it either way. Convenient to have the same test harness work for all function types but that only holds if the test methods needed are meaningfully similar.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@TheIronDev
Copy link
Contributor Author

@inlined Providing a default data might fix that then? I'll follow up with you on that.

This cl decouples the implementation logic from wrapping V1 functions
and places it into a separate directory.

I went back and re-exported the same exports from v1 back out to main.ts
to reduce disruption from existing usage.
Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

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

I suspect you'll have an easier time if you use a strategy pattern. For example, you can look up a Service (e.g. Pub/Sub or GCS) given an eventType and then call into that service's "generateMockEvent" method. This will help you, for example, only insert params on RTDB events or to insert appId as a field in FireAlerts events.

We're possibly going to have a lot of event types given that 3P events are coming. I think a little over-engineering to have a codebase where changes are decoupled from the rest of the codebase and have an obvious way to grow organically would be in our best interest.


if (
has(cloudFunction, '__trigger.httpsTrigger') &&
get(cloudFunction, '__trigger.labels.deployment-callable') !== 'true'
Copy link
Member

Choose a reason for hiding this comment

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

Note: this is going to be __endpoint.callableTrigger in the endpoint world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh neat, didn't realize v1 shared the same ManifestEndpoint.

I fear that changing the logic here will break existing usages (if people mocked out their own event with httpsTrigger), but maybe a major bump is the right time to be making a realignment to match spec...

);
}

function _checkOptionValidity(
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind if, in the future, we try to solve these sorts of concerns with the type system rather than with helper functions (though having both is great too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, in the future type system checking would be better.

CloudEvent stuff is being decoupled shortly, stay tuned!™
@TheIronDev
Copy link
Contributor Author

I suspect you'll have an easier time if you use a strategy pattern. For example, you can look up a Service (e.g. Pub/Sub or GCS) given an eventType and then call into that service's "generateMockEvent" method. This will help you, for example, only insert params on RTDB events or to insert appId as a field in FireAlerts events.

We're possibly going to have a lot of event types given that 3P events are coming. I think a little over-engineering to have a codebase where changes are decoupled from the rest of the codebase and have an obvious way to grow organically would be in our best interest.

I agree with the strategy pattern. We need a little more signal to differentiate some of the alerts, so I'm planning on also using eventTrigger?.eventFilters as well.

Will ping you when I get closer to cleaning everything up.

This commit introduces MockCloudEvent.

Its very apparent the switch-case shenanigans are too brittle to
handle the growing number of CloudEvents. So Instead, I'm
introducing a wrapper that contains most of the implementation
within `src/mock-cloud-event-partial-definitions.ts`.

The wrapper uses a `match` function to see which implementation of
MockCloudEventPartial to use, and then uses a `generatePartial`
to get the expected Partial<CloudEvent>.

This is not particularly DRY, but it seemed more useful to
keep the building blocks basic rather than being too clever. I'm
happy to be convinced otherwise though.

I'll be breaking / updating other unit tests momentarily.
This commit refactors the V2 wrap function, removing the need for
the end-user to generate a mock event to pass into the function.

The spec tests are still mostly useless, and we are still missing
data.

I wanted to finish this refactor before implementing the data potion,
otherwise it would be more work to clean up.
@TheIronDev
Copy link
Contributor Author

TheIronDev commented Apr 29, 2022

Alright, Storage data is done!
Still working on generating data for....

  • alerts
  • pubsub
  • eventarc

Sorry, something went wrong.

This is a quality-of-life improvement that splits up the giant
`src/mock-cloud-event-partial-definitions.ts` by event type.

`src/mock-cloud-event-partial-definitions.ts` is now just `partials.ts`,
and it returns a list of partials that are all definined in different
files.

Its not the DRYest thing in thr world... but its much more readable.
src/v2.ts Outdated
throw new Error('This function can only wrap V2 CloudFunctions.');
}

return (cloudEvent: CloudEvent) => cloudFunction.run(cloudEvent);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to be splitting the mock-cloud-event into its own file, so the update will be coming shortly.

);
}

function _checkOptionValidity(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, in the future type system checking would be better.

@TheIronDev TheIronDev requested a review from inlined May 2, 2022 20:44
Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

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

I think this is fine for a checkpoint. I may dig in later to play with the type system, DRY things out, and/or improve the way one field can be generated from another.

expect(cloudEvent.source).equal(
'//firebasealerts.googleapis.com/projects/__PROJECT_ID__');
expect(cloudEvent.subject).equal(undefined);
});
Copy link
Member

Choose a reason for hiding this comment

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

and eventually tests on data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack (will follow up!)

expect(cloudEvent.source).equal(
`//storage.googleapis.com/projects/_/buckets/${bucketName}`);
expect(cloudEvent.subject).equal('objects/file_name');
});
Copy link
Member

Choose a reason for hiding this comment

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

and another test for when bucketName is not specified

it('should update CloudEvent appropriately', () => {
const cloudFn = alerts.onAlertPublished('alertType', handler);
const cloudFnWrap = wrapV2(cloudFn);
expect(cloudFnWrap().cloudEvent).to.include({});
Copy link
Member

Choose a reason for hiding this comment

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

Should I assume these tests come later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, these events will get updated later 👍

function getOnAlertPublishedData(): FirebaseAlertData<any> {
const now = new Date().toISOString();
return ({
// '@type': 'type.googleapis.com/google.events.firebase.firebasealerts.v1.AlertData',

Choose a reason for hiding this comment

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

+1. Why is this commented out?

TheIronDev added 5 commits May 2, 2022 23:21
`<FunctionType, EventType>` -> `<EventType>`
Removing the hardcoded json from the partial. There needs to be a better pattern.
Stay tuned ™
@TheIronDev
Copy link
Contributor Author

I think this is fine for a checkpoint. I may dig in later to play with the type system, DRY things out, and/or improve the way one field can be generated from another.

Sounds good to me! I'm going to submit to unblock the other team members.

@TheIronDev TheIronDev merged commit 0182a8a into firebase:master May 3, 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.

None yet

4 participants