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

Jump to bottom

INT-3349 - Upgrade Polly.JS #688

Merged
merged 2 commits into from Apr 18, 2022
Merged

INT-3349 - Upgrade Polly.JS #688

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a663060
INT-3349 - Upgrade Polly.JS
austinkelleher Apr 16, 2022
a86b396
Upgrade to `fs-extra@^10.1.0` that fixes regressions
austinkelleher Apr 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -35,5 +35,8 @@
"ts-jest": "^27.1.0",
"typescript": "^4.2.4"
},
"resolutions": {
"fs-extra": "^9.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See description for the reason for this

Copy link

Choose a reason for hiding this comment

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

Related to fs.realpath.native being undefined, causing import of fs-extra@10.0.0 to throw error.

Defensively patched in jprichardson/node-fs-extra#953 and released in fs-extra@10.1.0.

Might want to bump it up instead, maybe?

But still, why is fs.realpath.native undefined? That is a root cause to be investigated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peteriman Thanks a lot for putting a fix in for this issue and for following up here!

It's not clear to me why fs.realpath.native is undefined either. I may spend a little time trying to figure that out and get back to you if I find anything.

Thanks again!

Copy link

@lamweili lamweili Apr 19, 2022 •
edited

Choose a reason for hiding this comment

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

While fs-extra no longer throws error on require/import, if someone still uses fs.realpath.native (be it from fs-extra or from native fs) they would still face errors.

@austinkelleher You might want to console.log(typeof fs.realpath.native) at the first line of code as a pre-requisite, then the subsequently before and after every require/import to identify the offending dependency that "deleted" it.

},
"version": "0.0.0"
}
9 changes: 3 additions & 6 deletions packages/integration-sdk-cli/package.json
View file
Open in desktop
Expand Up @@ -38,16 +38,13 @@
},
"devDependencies": {
"@jupiterone/integration-sdk-private-test-utils": "^8.11.0",
"@pollyjs/adapter-node-http": "^5.1.1",
"@pollyjs/core": "^5.1.1",
"@pollyjs/persister-fs": "^5.1.1",
"@pollyjs/adapter-node-http": "^6.0.5",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Major version changes were considered and various testing utilities have been updated. CHANGELOG can be found here: https://github.com/Netflix/pollyjs/blob/master/CHANGELOG.md#600-2021-11-30

"@pollyjs/core": "^6.0.5",
"@pollyjs/persister-fs": "^6.0.5",
"@types/fs-extra": "^9.0.13",
"@types/js-yaml": "^4.0.3",
"@types/json-diff": "^0.5.1",
"@types/lodash": "^4.14.158",
"@types/pollyjs__adapter-node-http": "^2.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See description. These are no longer needed! 🥳

"@types/pollyjs__core": "^4.3.3",
"@types/pollyjs__persister": "^4.3.1",
"@types/vis": "^4.21.20",
"memfs": "^3.2.0",
"neo-forgery": "^2.0.0",
Expand Down
10 changes: 2 additions & 8 deletions packages/integration-sdk-cli/src/__tests__/cli-run-failure.test.ts
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@ import { createCli } from '../index';
import { setupSynchronizerApi } from './util/synchronization';

import * as log from '../log';
import { createTestPolly } from './util/recording';

jest.mock('../log');

Expand All @@ -37,14 +38,7 @@ beforeEach(() => {
process.env.JUPITERONE_ACCOUNT = 'mochi';
loadProjectStructure('validationFailure');

polly = new Polly('run-cli-failure', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small bonus change. Extracted this functionality into createTestPolly because it is used in 3 tests.

adapters: ['node-http'],
persister: 'fs',
logging: false,
matchRequestsBy: {
headers: false,
},
});
polly = createTestPolly('run-cli-failure');

mocked(mockCreateIntegrationLogger).mockReturnValue(
createIntegrationLogger({ name: 'test' }),
Expand Down
11 changes: 2 additions & 9 deletions packages/integration-sdk-cli/src/__tests__/cli-run.test.ts
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@ import {
} from '@jupiterone/integration-sdk-core';

import * as log from '../log';
import { createTestPolly } from './util/recording';

jest.mock('../log');

Expand All @@ -28,15 +29,7 @@ beforeEach(() => {
process.env.JUPITERONE_API_KEY = 'testing-key';
process.env.JUPITERONE_ACCOUNT = 'mochi';

polly = new Polly('run-cli', {
adapters: ['node-http'],
persister: 'fs',
logging: false,
matchRequestsBy: {
headers: false,
},
});

polly = createTestPolly('run-cli');
loadProjectStructure('typeScriptIntegrationProject');

jest.spyOn(process, 'exit').mockImplementation((code: number | undefined) => {
Expand Down
11 changes: 2 additions & 9 deletions packages/integration-sdk-cli/src/__tests__/cli-sync.test.ts
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@ import {
} from './util/synchronization';

import * as log from '../log';
import { createTestPolly } from './util/recording';

jest.mock('../log');

Expand All @@ -25,15 +26,7 @@ beforeEach(() => {
process.env.JUPITERONE_API_KEY = 'testing-key';
process.env.JUPITERONE_ACCOUNT = 'mochi';

polly = new Polly('sync-cli', {
adapters: ['node-http'],
persister: 'fs',
logging: false,
matchRequestsBy: {
headers: false,
},
});

polly = createTestPolly('sync-cli');
loadProjectStructure('synchronization');

jest.spyOn(process, 'exit').mockImplementation((code: number | undefined) => {
Expand Down
16 changes: 16 additions & 0 deletions packages/integration-sdk-cli/src/__tests__/util/recording.ts
View file
Open in desktop
@@ -0,0 +1,16 @@
import { Polly, PollyConfig } from '@pollyjs/core';

export function createTestPolly(
recordingName: string,
pollyConfigOverrides?: PollyConfig,
) {
return new Polly(recordingName, {
adapters: ['node-http'],
persister: 'fs',
logLevel: 'silent',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only change made when creating new Polly instances is here. There used to be a logging boolean, which was assigned false. In Polly.JS v6, logging was removed and replaced with logLevel. I kept the existing behavior by assigning silent as the value.

matchRequestsBy: {
headers: false,
},
...pollyConfigOverrides,
});
}
6 changes: 4 additions & 2 deletions packages/integration-sdk-cli/src/__tests__/util/synchronization.ts
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ export function setupSynchronizerApi({ polly, job, baseUrl }: SetupOptions) {
.post(`${baseUrl}/persister/synchronization/jobs/${job.id}/entities`)
.intercept((req, res) => {
allowCrossOrigin(req, res);
job.numEntitiesUploaded += JSON.parse(req.body).entities.length;
job.numEntitiesUploaded += JSON.parse(req.body!).entities.length;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new internal Polly.JS TypeScript definitions allow body to be undefined. For this test to pass, body would need to be defined.

res.status(200).json({ job });
});

Expand All @@ -37,7 +37,9 @@ export function setupSynchronizerApi({ polly, job, baseUrl }: SetupOptions) {
.post(`${baseUrl}/persister/synchronization/jobs/${job.id}/relationships`)
.intercept((req, res) => {
allowCrossOrigin(req, res);
job.numRelationshipsUploaded += JSON.parse(req.body).relationships.length;
job.numRelationshipsUploaded += JSON.parse(
req.body!,
).relationships.length;
res.status(200).json({ job });
});

Expand Down
10 changes: 3 additions & 7 deletions packages/integration-sdk-testing/package.json
View file
Open in desktop
Expand Up @@ -25,13 +25,9 @@
"dependencies": {
"@jupiterone/integration-sdk-core": "^8.11.0",
"@jupiterone/integration-sdk-runtime": "^8.11.0",
"@pollyjs/adapter-node-http": "^5.1.1",
"@pollyjs/core": "^5.1.1",
"@pollyjs/persister-fs": "^5.1.1",
"@types/har-format": "^1.2.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer needed since this data is exported from @pollyjs/persister! 🥳

"@types/pollyjs__adapter-node-http": "^2.0.1",
"@types/pollyjs__core": "^4.3.3",
"@types/pollyjs__persister": "^4.3.1",
"@pollyjs/adapter-node-http": "^6.0.5",
"@pollyjs/core": "^6.0.5",
"@pollyjs/persister-fs": "^6.0.5",
"deepmerge": "^4.2.2",
"lodash": "^4.17.15"
},
Expand Down
8 changes: 4 additions & 4 deletions packages/integration-sdk-testing/src/recording.ts
View file
Open in desktop
@@ -1,16 +1,16 @@
import { Har, Entry } from 'har-format';
import defaultsDeep from 'lodash/defaultsDeep';
import { gunzipSync } from 'zlib';

import NodeHttpAdapter from '@pollyjs/adapter-node-http';
import { Polly, PollyConfig } from '@pollyjs/core';
import FSPersister from '@pollyjs/persister-fs';
import { HarEntry, Har } from '@pollyjs/persister';

Polly.register(NodeHttpAdapter);
Polly.register(FSPersister);

export type Recording = Polly;
export type RecordingEntry = Entry;
export type RecordingEntry = HarEntry;

export interface SetupRecordingInput {
directory: string;
Expand Down Expand Up @@ -78,7 +78,7 @@ export function setupRecording({
return 'JupiterOneIntegationFSPersister';
}

saveRecording(recordingId: number, data: Har): void {
async onSaveRecording(recordingId: string, data: Har): Promise<void> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking change in Polly.JS v6. See the Polly.JS CHANGELOG.

data.log.entries.forEach((entry) => {
// Redact tokens, even though they expire
entry.request.headers = entry.request.headers.map((header) => ({
Expand All @@ -104,7 +104,7 @@ export function setupRecording({
mutateEntry?.(entry);
});

super.saveRecording(recordingId, data);
return super.onSaveRecording(recordingId, data);
}
}

Expand Down