refactor: replace archiver with @zip.js/zip.js #4769
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ctlai95
force-pushed
the
ctlai95/update-zip-stream
branch
5 times, most recently
from
01bdfa3 to
65ec6e8
Compare
Contributor
The "minimum" is testing a very old version of vscode, which has an older nodejs. I think we can and should bump the minimum to a vscode version that has node 16. That will likely need to be a separate PR and may require some minor fixups in the tests, etc. |
ctlai95
force-pushed
the
ctlai95/update-zip-stream
branch
7 times, most recently
from
1c2dea8 to
2556aee
Compare
ctlai95
force-pushed
the
ctlai95/update-zip-stream
branch
from
2556aee to
6f9e995
Compare
Contributor
|
I've added #4787 to our current sprint, to unblock this. So please feel free to continue with the assumption that the vscode minimum will be bumped. |
ctlai95
force-pushed
the
ctlai95/update-zip-stream
branch
from
b41d29a to
e7c95ae
Compare
ctlai95
force-pushed
the
ctlai95/update-zip-stream
branch
from
4be3f1a to
7532d82
Compare
justinmk3
reviewed
May 8, 2024
pbalogh
reviewed
May 13, 2024
| export interface ZipStreamResult { | ||
| sizeInBytes: number | ||
| md5: string | ||
| hash: string |
pbalogh
approved these changes
May 13, 2024
Contributor
ctlai95
changed the title
|
This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions. |
ctlai95
changed the title
Contributor
Author
|
/retryBuilds |
Contributor
|
Update: we will wait 1 more week before increasing min vscode to 1.83, see #5749 |
Contributor
Hweinstock
added a commit
that referenced
this pull request
Oct 23, 2024
…ystem counts (#5786) ## Problem Performance tests are currently flaky, making them a poor signal for performance regressions within the code base. Additionally, false alarms slow down development of unrelated features as test failures keep CI from being "green". Therefore, rather than relying only on system usage thresholds for performance regressions, we can count the number of high-risk / potentially slow operations made by the code as a deterministic measure of its performance. However, directly counting each individual potentially slow operation used within the performance tests highly couples the test to the implementation details, making the tests less effective if the implementation changes. Therefore, the goal of this PR is the following: 1. decrease performance test flakiness by increasing thresholds. 2. increase performance test effectiveness by relying on deterministic measures. 3. avoid coupling the tests to the implementation details. ## Solution To meet goal (1), we increase the thresholds of the tests to decrease the changes of a false alarm. To meet goal (2), we count expensive operations. But, to avoid tying it to the implementation details, we count the expensive operations using somewhat-loose upper bounds. Thus, implementation changes modifying the exact number of expensive operations by a small constant do not set a false alarm. However, if they increase the number of expensive operations by a multiplicative factor, the upper bound will alert us. As an example, we don't want the test to fail if it makes 5-10 more file system calls when working with a few hundred files, but we do want the test to fail if it makes 2x the number of files system calls. Therefore, in the code the bounds are often described as "operations per file", since it is the multiplicative increase we are concerned about. This allows us to achieve goal (3). ### Implementation Details - The most common "expensive operation" we count is file system calls (through our `fs` module). Some other examples include the use of `zip` libraries or loading large files into memory. - We separate the upper bounds for the file system into `read` and `write` bounds. This granularity allows us to assert that specific code paths do not modify any files. ## Notes - `AdmZip` removed in #4769 , so we don't address spying on it here. Once `zip.js` is implemented, we can spy on it in a follow up. --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
Contributor
|
Can we now remove the dependencies that were added in #4629 ? |
Contributor
Author
|
@justinmk3 I can remove |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The
archiverpackage usesfswhich is not available in the browser. Runningnpm run testWebresults in the following error:Solution
archiverwith@zip.js/zip.jswhich has browser support.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.