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

fix(unzip): migrate the dependency used to extract zip archives from unzipper to adm-zip #9

Conversation

sounisi5011
Copy link
Collaborator

@sounisi5011 sounisi5011 commented Jun 4, 2023

There is a bug in unzipper that does not extract files in a ZIP archive properly.
ZJONSSON/node-unzipper#271

So instead of unzipper, use adm-zip.

  • Fix bugs
  • Update tests for behavior changes

Closes #7

…er` package

Previous tests have mocked up the `Extract` function of the `unzipper` package.
This test does not confirm that the same functionality is maintained by updating the `src/assets/unzip.js` file.
It also does not test whether the previous code and the new code extracted the ZIP file correctly.

So I rewrote the test code.
By mocking the file system and the HTTP request, we can now do the same test no matter which unzip library we use.

Added dependencies:
+ mock-fs
+ nock

The rewritten test code was tested with the old `src/assets/unzip.js` before the update.
If necessary, check out this commit using the `git checkout` command and test the old `src/assets/unzip.js` with the updated test code.
…`unzipper` to `adm-zip`

There is a bug in "unzipper" that does not extract files in a ZIP archive properly.
So instead of "unzipper", use "adm-zip".
@sounisi5011
Copy link
Collaborator Author

sounisi5011 commented Jun 4, 2023

Note
In this pull request, the test code has been substantially rewritten.
I have run the updated tests against the source before and after the change. However, I recommend that you run the tests on your end as well, just to be sure.

  1. Run the git restore --source=master src/ command to revert the source code back to what it was before it was changed by this pull request.
  2. Run the npm ci command to update dependencies
  3. Run the npm i -D unzipper command to reinstall unzipper
  4. Execute the npm test ./__test__/assets/unzip.spec.js command to run the test
  5. After testing, run the git restore --source=HEAD src/ command to restore the source code
  6. Run the npm test ./__test__/assets/unzip.spec.js command again to confirm that the test was successful

The test code before my changes used Jest mock functions.
However, I rewrote it into very unreadable test code using the `Promise` constructor.

This is because initially I could not think of a way to write an asynchronous test that waited until the mock function was called.
But now I have found a way.

So I will modify the test code to make the differences smaller.
@sounisi5011
Copy link
Collaborator Author

I have modified the test code.

  it('should call onSuccess on unzip close', async () => {
    const req = await getReq(
      'https://example.com/releases/latest.zip',
      TEST_ZIP.data,
    );

    await expect(new Promise((onSuccess, onError) => {
      unzip({ opts: { binPath: './bin', binName: 'command' }, req, onSuccess, onError });
    })).resolves.not.toThrow();
  });

I modified these hard-to-read codes as follows:

  it('should call onSuccess on unzip close', async () => {
    const req = await getReq(
      'https://example.com/releases/latest.zip',
      TEST_ZIP.data,
    );
    const { onSuccess, onError, waitFinish } = createCallbacks();

    unzip({ opts: { binPath: './bin', binName: 'command' }, req, onSuccess, onError });

    await waitFinish;

    expect(onSuccess).toHaveBeenCalled();
  });

This will reduce the differences from the original test code before the change, and should make it easier to see what changes have been made.

@andreynering andreynering merged commit 4cb4b94 into go-task:master Jun 17, 2023
@sounisi5011 sounisi5011 deleted the fix-issue-7/migrate-dependency-used-to-extract-zip-archives-from-unzipper-to-adm-zip branch June 17, 2023 15:13
sounisi5011 added a commit to sounisi5011/package-version-git-tag that referenced this pull request Jul 2, 2023
The `@go-task/go-npm` included in `@go-task/cli` now installs the `task` command in the correct location on Windows!
see go-task/go-npm#5, go-task/go-npm#8, and go-task/go-npm#9

This reverts commit abb45e1
sounisi5011 added a commit to sounisi5011/package-version-git-tag that referenced this pull request Jul 2, 2023
* ⬆️ Update dependency @go-task/cli to v3.27.1 ( 77618ab )

* 🔥 Remove patches for `@go-task/go-npm` package ( d626085 )

    The `@go-task/go-npm` included in `@go-task/cli` now installs the `task` command in the correct location on Windows!
    see go-task/go-npm#5, go-task/go-npm#8, and go-task/go-npm#9

    This reverts commit abb45e1

* ⬆️ Update `pnpm-lock.yaml` ( 36c5f37 )

* 📝 Update CHANGELOG ( 766d804 )

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Sonishi Izuka <sounisi5011@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Incorrect binary files installed with Node.js 18.16.0 and later, Node.js 19.8.0 and later, and Node.js 20
2 participants