Skip to content

Conversation

@JJ
Copy link
Contributor

@JJ JJ commented Oct 4, 2022

✍️ 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

♥️ Thank you!

@JJ JJ changed the title [♻] {PROD4POD-1930} Move zipfile mock-up [♻] {PROD4POD-1930} Move zipfile mock-up and improve coverage Oct 4, 2022
@JJ JJ changed the title [♻] {PROD4POD-1930} Move zipfile mock-up and improve coverage [♻] {PROD4POD-1931} Move zipfile mock-up and improve coverage Oct 4, 2022
Copy link
Contributor

@fhd fhd left a comment

Choose a reason for hiding this comment

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

Generally LGTM - but it probably makes sense for Richard to take a look since he wrote at least some of it.


it("passes through non-string data", () => {
const foo42 = { foo: 42 };
expect(jsonStringifyWithUtfEscape(foo42)).toBe('{"foo":42}');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'd do: expect(jsonStringifyWithoutUtfEscape(foo42)).toBe(JSON.stringify(foo42)) - to be more clear that it's just a normal stringify without UTF-8 data. But no strong opinion, being explicit with the values is also good.

let status = null;

beforeAll(async () => {
let zipFile = new ZipFileMock();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: For variables that don't get reassigned, we generally used const (i.e. wherever possible).

@JJ JJ merged commit a580e09 into main Oct 14, 2022
@JJ JJ deleted the ♻-move-zipfile-mock-up branch October 14, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants