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

JJ · 2022-10-03T08:12:26Z

✍️ Description

Increase coverage of core tests. poly-importer is used by all features, and coverage was rather dim

The scope of this PR will be defined on the run. For the time being, it's 🚧 and tries to increase it beyond original 13.68%

🏗️ Fixes 1931

ℹ️ Other information

Refactoring will be made as needed. For starters, it needed configuration of the jest jsdom environment.

♥️ Thank you!

And set up properly jest to work in a jsdom environment

Up to 20% coverage now

Seems the right one for an abstract base class :white_check_mark: too

Added dep for testing

But this is going to need some more mock code, so will leave it like this

# ✍️ Description Assigning a value to an attribute violated encapsulation, which is easy to do in JS, but still. This is a cleaner API, with a private `changeListener` which is assigned when the object is built. ### 🏗️ Works with PROD4POD-1931 Still cleaning up and refactoring the importer, looking at improving coverage. Related to #1209 and #1204, but wanted it to be considered on its own. Also it's a change of API, which is better if considered on its own.

fhd

Pretty good start I'd say!

fhd · 2022-10-13T10:09:01Z

feature-utils/poly-import/src/errors/polyIn-errors.js

@@ -2,26 +2,24 @@ class PolyImportError extends Error {
    constructor(type, cause) {
        super(`Failed to ${type} file`);
        this.cause = cause;
+        this.name = this.constructor.name;


fhd · 2022-10-13T10:12:22Z

feature-utils/poly-import/test/storage.test.js

+    });
+});
+
+describe("Can remove archives ", () => {


Looking at the code - it doesn't remove any archives, does it? Isn't it more it("ignores non-existent archives")? (I also think this is more of an it to "File storage" rather than its own describe, but that part is a nit.)

It's quite incomplete at the moment, I'm afraid. But yes, this specific one could be renamed and instantiated as you say.

fhd · 2022-10-13T10:13:54Z

feature-utils/poly-import/utils/performance-telemetry.js

@@ -3,6 +3,7 @@
 *
 * @class
 */
+


Nit: We usually don't have empty lines between JSDoc comment and declaration.

# ✍️ Description The mock for zip-files is more useful in the common `poly-import` module. That way it can also be used in the module itself to mock zipfiles, and imported from any module that needs to mock zips. While we were at it, boost a bit coverage of said test. ### 🏗️ Fixes PROD4POD-1931 It is a precondition for #1204, since `Storage` needs zip files to be tested. ## ℹ️ Other information Some minor refactoring also made. Might include some refactoring of tests, mainly to reduce boilerplate. Not for the time being. ### Additional functionality * ✅ adds tests in parts that were not covered, for instance, file name analysis in Facebook

fhd · 2022-10-18T06:25:41Z

feature-utils/poly-import/test/storage.test.js

+    });
+});
+
+describe("File storage", () => {


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

fhd · 2022-10-18T06:26:15Z

feature-utils/poly-import/test/importer.test.js

@@ -0,0 +1,11 @@
+import { Importer } from "../src";
+
+describe("Importer can't be instantiated ", () => {


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

fhd · 2022-10-18T06:26:44Z

features/facebookImport/test/ministory/report-metadata.test.js

@@ -44,6 +44,7 @@ describe("Report metadata analysis", () => {
                ),
            ],
        ]);
+        console.log(zipFile);


Nit: Forgotten?

fhd · 2022-10-18T06:29:06Z

feature-utils/poly-import/test/storage.test.js

+        fileStorage = new FeatureFileStorage(pod);
+    });
+    it("ignores non-existent archives", () => {
+        expect(fileUri).toMatch(polyProtocolRegex);


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.

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.

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

fhd · 2022-10-18T06:30:26Z

platform/feature-api/api/src/pod-api/uri.ts

-const polyProtocol = "polypod://";
-const polyProtocolRegex = new RegExp(`^${polyProtocol}`);
+export const polyProtocol = "polypod://";
+export const polyProtocolRegex = new RegExp(`^${polyProtocol}`);


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.

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.

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.

# ✍️ Description While doing #1204 a bit of refactoring is needed; and I realized this return value is never used. Giving it its own PR since it's a change in API, and want it to be tested with the rest. Also, some refactoring mainly eliminating `await` and `return` where they're not actually needed. ### 🏗️ Fixes PROD4PROD-1931 It contributes to cleaning the repository, and improve test coverage in `importer`; eliminating code that's not used will no doubt contribute to that.

# ✍️ Description While doing #1204, the need to refactor mocks arose. It became a bit hefty for an already heavy PR, so it's better taken outside. ### 🏗️ Working with PROD4POD-1931 ## ℹ️ Other information I'll leave it as draft for the time being, since some functions might be unneeded after this. Feedback welcome anyway.

feature-utils/poly-import/test/importer.test.js

feature-utils/poly-import/test/polyIn-errors.test.js

irstavr · 2022-10-31T08:51:50Z

feature-utils/poly-import/test/telemetry.test.js

+    it("Returns increasing elapsed time", () => {
+        const oldTime = telemetry.elapsedTime();
+        expect(oldTime).toBeGreaterThan(0);
+        expect(telemetry.elapsedTime()).toBeGreaterThan(oldTime);
+    });


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);

});

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

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

feature-utils/poly-import/test/storage.test.js

irstavr · 2022-10-31T09:01:09Z

feature-utils/poly-import/test/polyIn-errors.test.js

+        classData.forEach(([testClass, testMsg]) => {
+            const testCause = "test";


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)

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.

Co-authored-by: irstavr <[email protected]>

Add test for Telemetry

e0880a1

And set up properly jest to work in a jsdom environment

JJ marked this pull request as draft October 3, 2022 08:12

Juan Julián (JJ) Merelo added 6 commits October 3, 2022 10:26

Adds tests for errors

1ebcb7e

Up to 20% coverage now

Refactor to avoid DRY

0393c1f

♻️ and test actual cause

fb9c9f9

♻️ to TypeError

fc1ec4b

Seems the right one for an abstract base class :white_check_mark: too

🐛 it actually calls the function

6e3e41b

✅ storage tests started

e430caa

Added dep for testing

JJ mentioned this pull request Oct 4, 2022

[♻] {PROD4POD-1931} Move zipfile mock-up and improve coverage #1205

Merged

JJ changed the title ~~{PROD4POD-1930} Add tests for poly-importer~~ {PROD4POD-1931} Add tests for poly-importer Oct 4, 2022

Juan Julián (JJ) Merelo added 3 commits October 10, 2022 07:39

Merge branch 'main' into ♻-PROD4POD-1930

8a2b0af

DRY

fb307cd

✅ use destructuring

0543893

This was referenced Oct 10, 2022

[:coffin:] Refactoring return values #1209

Merged

[:recycle:] changeListener moved to the constructor #1210

Merged

Juan Julián (JJ) Merelo added 2 commits October 10, 2022 09:28

Merge branch 'main' into ♻-PROD4POD-1930

2c65515

Starting to test with archive

ec523e4

But this is going to need some more mock code, so will leave it like this

fhd reviewed Oct 13, 2022

View reviewed changes

Juan Julián (JJ) Merelo added 7 commits October 17, 2022 08:58

Merge branch 'main' into ♻-PROD4POD-1930

93a74b6

Address comments

20d23bf

🚨 via ✅

54c97d6

Export protocol regex to use in tests

7894305

Test URI appropriately ✅

483b003

Non-interface-changing new constructor ♻️

95d5309

Change to old interface

1bd60a1

JJ mentioned this pull request Oct 17, 2022

[♻] Improve ZipFileMock constructor #1218

Merged

Juan Julián (JJ) Merelo added 2 commits October 18, 2022 07:20

Merge branch 'main' into ♻-PROD4POD-1930

76910b8

Revert part of the change which is in #1218

b28b87c

JJ mentioned this pull request Oct 18, 2022

[✨] {PROD4POD-635} Mock archive-related methods #1222

Draft

fhd requested changes Oct 18, 2022

View reviewed changes

Merge branch 'main' into ♻-PROD4POD-1930

15245f0

⚔️

e991335

irstavr reviewed Oct 31, 2022

View reviewed changes

JJ and others added 3 commits October 31, 2022 10:10

Update feature-utils/poly-import/test/importer.test.js

fac7da3

Co-authored-by: irstavr <[email protected]>

Update feature-utils/poly-import/test/polyIn-errors.test.js

5ac68ff

Co-authored-by: irstavr <[email protected]>

Update feature-utils/poly-import/test/storage.test.js

e1979e1

Co-authored-by: irstavr <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JJ commented Oct 3, 2022 •

edited

Loading

fhd left a comment

fhd Oct 13, 2022

fhd Oct 13, 2022

JJ Oct 17, 2022

fhd Oct 13, 2022

fhd Oct 18, 2022

fhd Oct 18, 2022

fhd Oct 18, 2022

fhd Oct 18, 2022

JJ Oct 18, 2022

fhd Oct 18, 2022

fhd Oct 18, 2022

JJ Oct 18, 2022

fhd Oct 18, 2022

irstavr Oct 31, 2022

JJ Oct 31, 2022

irstavr Oct 31, 2022

irstavr Oct 31, 2022

JJ Oct 31, 2022

		@@ -0,0 +1,11 @@
		import { Importer } from "../src";

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

		classData.forEach(([testClass, testMsg]) => {
		const testCause = "test";

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

Are you sure you want to change the base?

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

Conversation

JJ commented Oct 3, 2022 • edited Loading

✍️ Description

🏗️ Fixes 1931

ℹ️ Other information

fhd 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

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

JJ commented Oct 3, 2022 •

edited

Loading