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.

Sign up for GitHub

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

Improve types for mocking #3082

Merged
merged 7 commits into from
Oct 7, 2024
Merged

Improve types for mocking #3082

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3e8e71a
Improve types for mocking
oleg-codaio Oct 3, 2024
1fc04a2
Fix untransformBody for arrays
oleg-codaio Oct 7, 2024
5c3745f
Remove executeGetPermissionsFormulaFromPackDef
oleg-codaio Oct 7, 2024
fef0713
Update changelog
oleg-codaio Oct 7, 2024
3bcf351
Rebuild
oleg-codaio Oct 7, 2024
96ccb7b
Fix validation for non object types
oleg-codaio Oct 7, 2024
728a745
Improve changelog
oleg-codaio Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ This changelog keeps track of all changes to the Packs SDK. We follow convention

- Add continuation for get permissions request and response

### Changed

- Improve types for testing
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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, 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, updated the changelog.

- Fix `untransformBody` helper for array inputs

## [1.7.19] - 2024-09-27

### Changed
Expand Down
4 changes: 2 additions & 2 deletions dist/handler_templates.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 11 additions & 6 deletions dist/testing/execution.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 8 additions & 4 deletions dist/testing/mocks.d.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion dist/testing/mocks.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions docs/reference/sdk/interfaces/testing.MockExecutionContext.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions docs/reference/sdk/interfaces/testing.MockSyncExecutionContext.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions handler_templates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ function mapKeys(obj: {[key: string]: any}, schema?: Schema): object {
continue;
}
remappedObject[newKey] = mappedKeys.length > 1 ? deepCopy(obj[key]) : obj[key];
const keySchema: Schema & ObjectSchemaProperty | undefined = schema.properties[newKey];
const keySchema: (Schema & ObjectSchemaProperty) | undefined = schema.properties[newKey];
const currentValue = remappedObject[newKey];
if (Array.isArray(currentValue) && isArray(keySchema) && isObject(keySchema.items)) {
remappedObject[newKey] = currentValue.map(val => mapKeys(val, keySchema.items));
Expand Down Expand Up @@ -367,7 +367,7 @@ function unmapKeys(obj: {[key: string]: any}, schema?: Schema): object {
continue;
}
remappedObject[newKey] = deepCopy(obj[key]);
const keySchema: Schema & ObjectSchemaProperty | undefined = schema.properties[key];
const keySchema: (Schema & ObjectSchemaProperty) | undefined = schema.properties[key];
const currentValue = remappedObject[newKey];
if (Array.isArray(currentValue) && isArray(keySchema) && isObject(keySchema.items)) {
remappedObject[newKey] = currentValue.map(val => unmapKeys(val, keySchema.items));
Expand All @@ -380,8 +380,8 @@ function unmapKeys(obj: {[key: string]: any}, schema?: Schema): object {

export function untransformBody(body: any, schema: Schema | undefined): any {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (isArray(schema) && isObject(schema.items)) {
const objectBody = body as Record<string, any>;
const mappedObjs = unmapKeys(objectBody, schema.items);
const objects = body as Array<Record<string, any>>;
const mappedObjs = objects.map(obj => unmapKeys(obj, schema.items));
return mappedObjs;
}

Expand Down
10 changes: 7 additions & 3 deletions test/compile_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {compilePackBundle} from '../testing/compile';
import {executeFormulaOrSyncWithVM} from '../testing/execution';
import {newMockSyncExecutionContext} from '../testing/mocks';
import path from 'path';
import sinon from 'sinon';
import {translateErrorStackFromVM} from '../runtime/common/source_map';

describe('compile', () => {
Expand Down Expand Up @@ -79,9 +80,12 @@ describe('compile', () => {
});
assert.equal(response, 'okay');
assert.isTrue(
executionContext.temporaryBlobStorage.storeBlob.calledWithMatch((buffer: any) => {
return Buffer.isBuffer(buffer);
}, 'text/html'),
executionContext.temporaryBlobStorage.storeBlob.calledWithMatch(
sinon.match((buffer: any) => {
return Buffer.isBuffer(buffer);
}),
'text/html',
),
);
});
});
8 changes: 4 additions & 4 deletions test/execution_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ describe('Execution', () => {
describe('fetcher mocks', () => {
it('fetch calls are mocked', async () => {
const context = newMockExecutionContext();
context.fetcher.fetch.returns(newJsonFetchResponse({result: 'hello'}));
context.fetcher.fetch.resolves(newJsonFetchResponse({result: 'hello'}));
const result = await executeFormulaFromPackDef(fakePack, 'Lookup', ['foo'], context);
assert.equal(result, 'hello');

Expand Down Expand Up @@ -177,8 +177,8 @@ describe('Execution', () => {
});

const context = newMockExecutionContext();
context.temporaryBlobStorage.storeBlob.returns('blob-url-1');
context.temporaryBlobStorage.storeUrl.returns('blob-url-2');
context.temporaryBlobStorage.storeBlob.resolves('blob-url-1');
context.temporaryBlobStorage.storeUrl.resolves('blob-url-2');
const result = await executeFormulaFromPackDef(fakeBlobPack, 'Blobs', [], context);
assert.equal(result, 'blob-url-1,blob-url-2');

Expand Down Expand Up @@ -391,7 +391,7 @@ describe('Execution', () => {
it('executes getConnectionName formula', async () => {
assertCondition(fakePackWithMetadata.defaultAuthentication?.type === AuthenticationType.HeaderBearerToken);
const context = newMockExecutionContext();
context.fetcher.fetch.returns(newJsonFetchResponse({username: 'some-user'}));
context.fetcher.fetch.resolves(newJsonFetchResponse({username: 'some-user'}));

const result = await executeMetadataFormula(
fakePackWithMetadata.defaultAuthentication.getConnectionName!,
Expand Down
68 changes: 44 additions & 24 deletions test/handler_templates_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ describe('handler templates', () => {
});

it('array schema', () => {
const inputBody = {Foo: 'bar', Baz: 2};
const inputBody = [{Foo: 'bar', Baz: 2}];
const body = untransformBody(inputBody, {
type: ValueType.Array,
items: {
Expand All @@ -118,10 +118,12 @@ describe('handler templates', () => {
},
},
});
assert.deepEqual(body, {
foo: 'bar',
baz: 2,
});
assert.deepEqual(body, [
{
foo: 'bar',
baz: 2,
},
]);
});

it('object schema', () => {
Expand All @@ -146,20 +148,26 @@ describe('handler templates', () => {
properties: {
Foo: {type: ValueType.String, fromKey: 'foo'},
Baz: {type: ValueType.Number, fromKey: 'baz'},
Biz: {type: ValueType.Array, fromKey: 'biz', items: {
type: ValueType.Object,
properties: {
Buzz: {type: ValueType.Boolean, fromKey: 'buzz'},
Biz: {
type: ValueType.Array,
fromKey: 'biz',
items: {
type: ValueType.Object,
properties: {
Buzz: {type: ValueType.Boolean, fromKey: 'buzz'},
},
},
}},
},
},
});
assert.deepEqual(body, {
foo: 'bar',
baz: 2,
biz: [{
buzz: false,
}],
biz: [
{
buzz: false,
},
],
});
});

Expand All @@ -170,9 +178,13 @@ describe('handler templates', () => {
properties: {
Foo: {type: ValueType.String, fromKey: 'foo'},
Baz: {type: ValueType.Number, fromKey: 'baz'},
Biz: {type: ValueType.Object, fromKey: 'biz', properties: {
Buzz: {type: ValueType.Boolean, fromKey: 'buzz'},
}},
Biz: {
type: ValueType.Object,
fromKey: 'biz',
properties: {
Buzz: {type: ValueType.Boolean, fromKey: 'buzz'},
},
},
},
});
assert.deepEqual(body, {
Expand Down Expand Up @@ -226,12 +238,16 @@ describe('handler templates', () => {
properties: {
Foo: {type: ValueType.String, fromKey: 'foo'},
Baz: {type: ValueType.Number, fromKey: 'baz'},
Biz: {type: ValueType.Array, fromKey: 'biz', items: {
type: ValueType.Object,
properties: {
Buzz: {type: ValueType.Boolean, fromKey: 'buzz'},
Biz: {
type: ValueType.Array,
fromKey: 'biz',
items: {
type: ValueType.Object,
properties: {
Buzz: {type: ValueType.Boolean, fromKey: 'buzz'},
},
},
}},
},
},
});
assert.deepEqual(body, ['foo', 'baz', 'biz']);
Expand All @@ -244,9 +260,13 @@ describe('handler templates', () => {
properties: {
Foo: {type: ValueType.String, fromKey: 'foo'},
Baz: {type: ValueType.Number, fromKey: 'baz'},
Biz: {type: ValueType.Object, fromKey: 'biz', properties: {
Buzz: {type: ValueType.Boolean, fromKey: 'buzz'},
}},
Biz: {
type: ValueType.Object,
fromKey: 'biz',
properties: {
Buzz: {type: ValueType.Boolean, fromKey: 'buzz'},
},
},
},
});
assert.deepEqual(body, ['foo', 'baz', 'biz']);
Expand Down
Loading