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.

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

{PROD4POD-1931} Add tests for poly-importer #1204

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
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
58 changes: 57 additions & 1 deletion feature-utils/poly-import/package-lock.json

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

10 changes: 10 additions & 0 deletions feature-utils/poly-import/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,15 @@
"scripts": {
"build": "rollup -c rollup.config.js",
"test": "jest --coverage"
},
"jest": {
"verbose": true,
"transform": {
"\\.[jt]sx?$": "babel-jest"
},
"testEnvironment": "jsdom"
},
"devDependencies": {
"@polypoly-eu/api": "file:../../platform/feature-api/api"
}
}
4 changes: 1 addition & 3 deletions feature-utils/poly-import/src/errors/polyIn-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,24 @@ class PolyImportError extends Error {
constructor(type, cause) {
super(`Failed to ${type} file`);
this.cause = cause;
this.name = this.constructor.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

}
}

export class FileImportError extends PolyImportError {
constructor(cause) {
super("import", cause);
this.name = "FileImportError";
}
}

export class FileSelectionError extends PolyImportError {
constructor(cause) {
super("select", cause);
this.name = "FileSelectionError";
}
}

export class RefreshFilesError extends PolyImportError {
constructor(cause) {
super("refresh", cause);
this.name = "RefreshFilesError";
}
}
2 changes: 1 addition & 1 deletion feature-utils/poly-import/src/importer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Telemetry } from "../utils/performance-telemetry";

export class Importer {
async import({ zipFile, dataAccount }) {
throw new Error(
throw new TypeError(
`Calling abstract base class with ${zipFile}, ${dataAccount}`
);
}
Expand Down
11 changes: 11 additions & 0 deletions feature-utils/poly-import/test/importer.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Importer } from "../src";

describe("Importer can't be instantiated ", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: What's with the trailing space here in the string? Also below.

it("throws a TypeError", async () => {
try {
await new Importer().import({ zipFile: null, dataAccount: null });
} catch (e) {
expect(e).toBeInstanceOf(TypeError);
}
});
});
23 changes: 23 additions & 0 deletions feature-utils/poly-import/test/polyIn-errors.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { FileImportError, FileSelectionError, RefreshFilesError } from "../src";

const classData = [
[FileImportError, "import"],
[FileSelectionError, "select"],
[RefreshFilesError, "refresh"],
];

describe("Errors have the right API ", () => {
it("throw correctly their type errors", () => {
classData.forEach(([testClass, testMsg]) => {
const testCause = "test";
Comment on lines +11 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

as this checks the classData (defined here and not in the real implementation), it seems to me it creates more tech-debt and potential issues if Error types are refactored at some point or more are added.
We might want to fill this object dynamically from PolyImportError somehow and not on our own? (or any other alternative)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it checks that it's effectively closed for modification, as in the open/closed principle. I'll check anyway if it can be improved through encapsulation. And any way thanks for the suggestion, since this is still a draft.

try {
throw new testClass(testCause);
} catch (error) {
expect(error).toBeInstanceOf(testClass);
expect(error.message).toMatch(new RegExp(testMsg));
expect(error.name).toBe(new testClass().constructor.name);
expect(error.cause).toBe(testCause);
}
});
});
});
28 changes: 28 additions & 0 deletions feature-utils/poly-import/test/storage.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { polyProtocolRegex } from "@polypoly-eu/api";
import { MockPod } from "@polypoly-eu/api/dist/mock-pod";
import { FeatureFileStorage } from "../src";

describe("File storage ", () => {
let fileStorage;
beforeAll(() => {
const pod = new MockPod();
fileStorage = new FeatureFileStorage(pod);
});
it("when instantiated there are no files stored", () => {
expect(fileStorage.files).toBeNull;
});
});

describe("File storage", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Figure these two describe blocks could be merged, but not that important.

let fileStorage;
let fileUri;
beforeAll(async () => {
const pod = new MockPod();
fileUri = await pod.polyOut.importArchive("noRealFile.zip");
fileStorage = new FeatureFileStorage(pod);
});
it("ignores non-existent archives", () => {
expect(fileUri).toMatch(polyProtocolRegex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why export the regex and test it? I think it'd be better to leave it as an implementation detail and write a dedicated test for handling of invalid URLs. That regex is never used directly in production code, so no point testing it directly IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, testing the fileUri against the protocol. The problem with leaving it as an implementation details is that we might end up having incompatible implementation details, and causing issues such as this one https://jira.polypoly.eu/browse/PROD4POD-1133 So it's not so much an implementation details as a single source of truth, because eventually we will be interchanging files between different implementations.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a proposed API change - quite a different story. If we make that change we can test it, because the API would guarantee us to local paths. Right now it gives us internal URLs. You can test that it's a valid URL, but that's really as far as we should go. Testing that some internal regex used in the code matches the output makes little sense to me. What if the regex is wrong, and that causes the issue? I'd be OK with duplicating the regex in the tests, that way we at least don't have to expose implementation details...

expect(fileStorage.files).toBeNull;
});
});
14 changes: 14 additions & 0 deletions feature-utils/poly-import/test/telemetry.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { Telemetry } from "../src";

describe("Telemetry measures performance ", () => {
let telemetry;
beforeAll(() => {
telemetry = new Telemetry();
});

it("Returns increasing elapsed time", () => {
const oldTime = telemetry.elapsedTime();
expect(oldTime).toBeGreaterThan(0);
expect(telemetry.elapsedTime()).toBeGreaterThan(oldTime);
});
Comment on lines +9 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just for its readability

Suggested change
it("Returns increasing elapsed time", () => {
const oldTime = telemetry.elapsedTime();
expect(oldTime).toBeGreaterThan(0);
expect(telemetry.elapsedTime()).toBeGreaterThan(oldTime);
});
test("elapsedTime() returns increasing elapsed time", () => {
const oldTime = telemetry.elapsedTime();
expect(oldTime).toBeGreaterThan(0);
const newTime = telemetry.elapsedTime();
expect(newTime).toBeGreaterThan(oldTime);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really... In this case I don't think readability would add much to it, just verbosity.

Copy link
Contributor

Choose a reason for hiding this comment

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

mostly meant it for the test title - not just the separated var for newTime...

});
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ describe("Report metadata analysis", () => {
),
],
]);
console.log(zipFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Forgotten?

const { facebookAccount, analysisResult } = await runAnalysisForExport(
ReportMetadataAnalysis,
zipFile
Expand Down
4 changes: 2 additions & 2 deletions platform/feature-api/api/src/pod-api/uri.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ export function createUUID(): string {
return uuidv4();
}

const polyProtocol = "polypod://";
const polyProtocolRegex = new RegExp(`^${polyProtocol}`);
export const polyProtocol = "polypod://";
export const polyProtocolRegex = new RegExp(`^${polyProtocol}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

See above - I'm not a fan of exposing things for test purposes, the ability to mock components not under test being the only exception. Otherwise, things should be as private as possible and tests should make an effort to test code paths through the public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really for testing; maybe I should have factored out this change as I have done with the rest. Main intention here is to make the polyProtocol a single source for all the literals that are used all over.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just realised that you could just call isPolypodUri with no need to export these internals.

That said, I'm still not sure why the unit tests for poly-import need to ensure that importArchive works as expected... They can't even, only in the test feature can we test API across platforms. The poly-import tests are for testing code in this library, nothing outside it.


/**
*
Expand Down