-
Notifications
You must be signed in to change notification settings - Fork 62
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
Improve the test results for Integrations internals #2376
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
/** | ||
* Useful for asserting results are okay, while still having access to error information. A context | ||
* object can be supplied to help provide context if the value of the result doesn't contain enough | ||
* information to know what went wrong. | ||
*/ | ||
export const expectOkResult = (result: Result<unknown>, context?: string | object) => { | ||
const labeled = { | ||
...result, | ||
context, | ||
}; | ||
expect(labeled).toEqual({ | ||
ok: true, | ||
context, | ||
value: expect.anything(), | ||
}); | ||
}; | ||
|
||
/** | ||
* Validate an error result is correctly returned. A context object can be supplied to help provide | ||
* context if the value of the result doesn't contain enough information to know what went wrong. | ||
*/ | ||
export const expectErrorResult = (result: Result<unknown>, context?: string | object) => { | ||
const labeled = { | ||
...result, | ||
context, | ||
}; | ||
expect(labeled).toEqual({ | ||
ok: false, | ||
context, | ||
error: expect.anything(), | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ import path from 'path'; | |
import * as fs from 'fs/promises'; | ||
import { deepCheck } from '../repository/utils'; | ||
import { FileSystemDataAdaptor } from '../repository/fs_data_adaptor'; | ||
import { expectOkResult } from './custom_expects'; | ||
|
||
const repository: TemplateManager = new TemplateManager([ | ||
new FileSystemDataAdaptor(path.join(__dirname, '../__data__/repository')), | ||
|
@@ -31,40 +32,57 @@ describe('The local repository', () => { | |
} | ||
// Otherwise, all directories must be integrations | ||
const integ = new IntegrationReader(integPath); | ||
await expect(integ.getConfig()).resolves.toMatchObject({ ok: true }); | ||
const config = await integ.getConfig(); | ||
expectOkResult(config, { integration: integ.name }); | ||
}) | ||
); | ||
}); | ||
|
||
it('Should pass deep validation for all local integrations.', async () => { | ||
const integrations: IntegrationReader[] = await repository.getIntegrationList(); | ||
await Promise.all( | ||
integrations.map(async (i: IntegrationReader) => { | ||
const result = await deepCheck(i); | ||
if (!result.ok) { | ||
console.error(i.directory, result.error); | ||
} | ||
expect(result.ok).toBe(true); | ||
integrations.map(async (integ: IntegrationReader) => { | ||
const result = await deepCheck(integ); | ||
expectOkResult(result, { integration: integ.name }); | ||
}) | ||
); | ||
}); | ||
}); | ||
|
||
// Nginx and VPC are specifically used in other tests, so we add dedicated checks for them. | ||
|
||
describe('Local Nginx Integration', () => { | ||
it('Should serialize without errors', async () => { | ||
const integration = await repository.getIntegration('nginx'); | ||
|
||
await expect(integration?.serialize()).resolves.toHaveProperty('ok', true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean all this while tests were actually working? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That file is also related by virtue of it being the same sort of result validation. We could use the same helpers there as well if we touch the file again. I didn't migrate everything since there's probably a lot of migration to do and the exact syntax for how I checked it in different places isn't consistent (which goes to show the necessity for a standard form...) |
||
expect(integration).not.toBeNull(); | ||
expectOkResult(await integration!.serialize()); | ||
}); | ||
|
||
it('Should serialize to include the config', async () => { | ||
it('Should contain its config in its serialized form', async () => { | ||
const integration = await repository.getIntegration('nginx'); | ||
const config = await integration!.getConfig(); | ||
const serialized = await integration!.serialize(); | ||
|
||
expect(serialized).toHaveProperty('ok', true); | ||
expect((serialized as { value: object }).value).toMatchObject( | ||
(config as { value: object }).value | ||
); | ||
expectOkResult(serialized); | ||
expect(serialized.value).toMatchObject(config.value!); | ||
}); | ||
}); | ||
|
||
describe('Local VPC Integration', () => { | ||
it('Should serialize without errors', async () => { | ||
const integration = await repository.getIntegration('amazon_vpc_flow'); | ||
|
||
expect(integration).not.toBeNull(); | ||
expectOkResult(await integration!.serialize()); | ||
}); | ||
|
||
it('Should contain its config in its serialized form', async () => { | ||
const integration = await repository.getIntegration('amazon_vpc_flow'); | ||
const config = await integration!.getConfig(); | ||
const serialized = await integration!.serialize(); | ||
|
||
expectOkResult(serialized); | ||
expect(serialized.value).toMatchObject(config.value!); | ||
}); | ||
}); |
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.
note: Another option is to extend expect directly
This saves having to tweak the lint config, but I felt the interface was too complicated. We ultimately don't need anything too sophisticated here.