Improve types for mocking #3082

oleg-codaio · 2024-10-03T02:30:30Z

This PR is part of a stack created with Aviator. Add support for validating permissions #3084 ➡️ Improve types for mocking #3082 `main`

Improving stubs in MockExecutionContext to be strongly typed.

Also adding a transformSyncFormulaResultToGetPermissionsRequest helper for writing tests working with permissions.

PTAL @coda/packs

oleg-codaio · 2024-10-03T02:31:20Z

testing/mocks.ts

@@ -30,7 +40,9 @@ export function newMockExecutionContext(overrides?: Partial<MockExecutionContext
    timezone: 'America/Los_Angeles',
    invocationToken: v4(),
    fetcher: {
-      fetch: sinon.stub(),
+      fetch: sinon.stub<[FetchRequest], Promise<FetchResponse>>().callsFake(async r => {
+        throw new Error(`Unhandled fetch: ${r.method} ${r.url}`);


Updating newMockExecutionContext to throw by default (rather than just return an empty value).

jonathan-codaio

CC @ekoleda-codaio

jonathan-codaio · 2024-10-03T16:24:55Z

CHANGELOG.md

@@ -4,6 +4,10 @@ This changelog keeps track of all changes to the Packs SDK. We follow convention

 ## [Unreleased]

+### Changed
+
+- Improve types for testing


The returns --> resolves thing seems like the most significant change, would call that out. Will external developers who have written unittests also need to update all usage like you did in the packs repo? If so I think we need to pre-announce this and explain how to update things. Clearly what was there before was broken so it's a good change but if it's not backwards-compatible we need to give a heads-up.

Yeah, if external developers wrote unit tests that didn't correctly return a promise (but instead returned a value), they'd need to update their tests to also use resolves to pass type checks. Functionally these tests still work because our code must in general use await rather than attempting to do promise chains with .then() - the latter would have caused RTEs otherwise.

@ekoleda-codaio can you help with the necessary announcement for this change?

@ekoleda-codaio I think the only developer-facing change to announce would be that if somebody previously wrote tests with newMockExecutionContext() and they used context.fetcher.fetch.returns(...) they need to update it to use context.fetcher.fetch.resolves(...). This should always have been the case but the old behavior was inaccurate considering these are async methods.

Thanks for clarifying, I missed this callout. Since tests could only be written with the CLI, and SDK version upgrades are manual and optional for the CLI (compared to the Pack Studio), I think it's fine to release this without an announcement or warning. I also suspect that the number of developers writing tests period is very small, so I expect the blast radius here to be incredibly small. A clear note in the Changelog is likely sufficient, but I can also write up a short community post once the version goes live.

Ok that's fine, let's just go with a more thorough changelog entry.

Ok, updated the changelog.

jonathan-codaio · 2024-10-03T16:27:30Z

testing/execution.ts

+ *
+ * @hidden
+ */
+export function transformSyncFormulaResultToGetPermissionsRequest(


Ah this is unfortunate. This is because when you call a sync formula in a test you get back normalized values?

Originally when we created the unittest framework, you got back pre-normalized values, which to me made the most sense because the pack maker is not supposed to care about normalization, it's a thing Coda does on their behalf but they don't really need to know or care about. At some point we made the unittest framework more robust or moved where normalization happens and as a side effect the return values started coming back normalized. So now developers do need to confront this detail.

It's probably worth seeing briefly whether there's a way to get back pre-normalized values during testing so that developers can just ignore this entirely. It might not be feasible though if things are tightly wound up in there.

Yeah, correct - when you call executeSyncFormulaFromPackDef or executeSyncFormulaFromPackDefSingleIteration that returns the normalized values. (There are some examples in the packs repo.) This happens in testing/execution.ts in findAndExecutePackFunction().

If we want to test permissions with the same values as passed in during ingestions then we need to untransform the result. However it looks like @alexd-codaio had a todo to revert these transforms:

packs-sdk/testing/execution.ts

Lines 95 to 96 in 0d5ab26

// TODO(alexd): Switch this to false or remove when we launch 1.0.0

useDeprecatedResultNormalization = true,

However if we bypass transforms then extraneous properties aren't filtered out, and we also have to set validateResult: false since that seems broken too. So there's a bit of work, we might want to transform and untransform back implicitly (perhaps add a new flag).

Oh good find. Ignoring extraneous properties for a second, could you get by without transformSyncFormulaResultToGetPermissionsRequest() if you just used useDeprecatedResultNormalization = false?

That said, transforming and untransforming I think does simulate what happens in coda, so it seems reasonable to do that and not expose an untransformer. I'd rather we just put that behavior behind useDeprecatedResultNormalization, use it explicitly for now, and then announce that we're changing the default.

Err sorry, I guess transform + untransform would become the behavior when useDeprecatedResultNormalization is false. You would explicitly set it to false in your new tests for now, and then we can make it part of this announcement and then change the default.

Sounds good, let me update. And yeah easier to work with than dealing with a new helper function, thanks for pointing me here to have another look at the code.

Updated! Using this in https://github.com/coda/packs/pull/4494.

oleg-codaio · 2024-10-07T05:00:03Z

handler_templates.ts

@@ -380,8 +380,8 @@ function unmapKeys(obj: {[key: string]: any}, schema?: Schema): object {

 export function untransformBody(body: any, schema: Schema | undefined): any {


Looks like we had a bug here compared to what transformBody does (cc @chris-codaio).

How does this manifest itself? Will this break any existing behavior related to 2-way sync?

Wasn't able to repro that code path with 2-way sync but I did update one place where we have test coverage here: https://github.com/coda/coda/pull/118340

ekoleda-codaio · 2024-10-07T18:08:04Z

LGTM. I'm not terribly clear on what the delta is for the Pack developer, but I don't see anything to object over. I'm not familiar enough with the implementation to approve the PR, so leaving that to others.

jonathan-codaio

Ok would just flesh out the changelog though re context.fetcher.fetch.resolves() so that it's actionable for external developers.

oleg-codaio requested a review from a team as a code owner October 3, 2024 02:30

oleg-codaio commented Oct 3, 2024

View reviewed changes

oleg-codaio force-pushed the osv-types-mocking branch from 9c91798 to 940a6dd Compare October 3, 2024 02:48

jonathan-codaio reviewed Oct 3, 2024

View reviewed changes

oleg-codaio force-pushed the osv-types-mocking branch from 9c68d8a to 279c7c9 Compare October 7, 2024 04:57

oleg-codaio commented Oct 7, 2024

View reviewed changes

oleg-codaio mentioned this pull request Oct 7, 2024

Add support for validating permissions #3084

Closed

oleg-codaio added 6 commits October 7, 2024 11:14

Improve types for mocking

3e8e71a

Fix untransformBody for arrays

1fc04a2

Remove executeGetPermissionsFormulaFromPackDef

5c3745f

Update changelog

fef0713

Rebuild

3bcf351

Fix validation for non object types

96ccb7b

oleg-codaio force-pushed the osv-types-mocking branch from db96032 to 96ccb7b Compare October 7, 2024 18:14

jonathan-codaio approved these changes Oct 7, 2024

View reviewed changes

Improve changelog

728a745

oleg-codaio merged commit 8422fa6 into main Oct 7, 2024
1 check passed

oleg-codaio deleted the osv-types-mocking branch October 7, 2024 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve types for mocking #3082

Improve types for mocking #3082

oleg-codaio commented Oct 3, 2024 •

edited

Loading

oleg-codaio Oct 3, 2024

jonathan-codaio left a comment

jonathan-codaio Oct 3, 2024

oleg-codaio Oct 3, 2024

jonathan-codaio Oct 7, 2024

ekoleda-codaio Oct 7, 2024

jonathan-codaio Oct 7, 2024

oleg-codaio Oct 7, 2024

jonathan-codaio Oct 3, 2024

oleg-codaio Oct 3, 2024

jonathan-codaio Oct 3, 2024

jonathan-codaio Oct 3, 2024

oleg-codaio Oct 3, 2024

oleg-codaio Oct 7, 2024 •

edited

Loading

oleg-codaio Oct 7, 2024

jonathan-codaio Oct 7, 2024

oleg-codaio Oct 7, 2024

ekoleda-codaio commented Oct 7, 2024

jonathan-codaio left a comment

	// TODO(alexd): Switch this to false or remove when we launch 1.0.0
	useDeprecatedResultNormalization = true,

		@@ -380,8 +380,8 @@ function unmapKeys(obj: {[key: string]: any}, schema?: Schema): object {

		export function untransformBody(body: any, schema: Schema \| undefined): any {

Improve types for mocking #3082

Improve types for mocking #3082

Conversation

oleg-codaio commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

jonathan-codaio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-codaio Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekoleda-codaio commented Oct 7, 2024

jonathan-codaio left a comment

Choose a reason for hiding this comment

oleg-codaio commented Oct 3, 2024 •

edited

Loading

oleg-codaio Oct 7, 2024 •

edited

Loading