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.

Sign up for GitHub

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

Jump to bottom

Updating upload.zip to be an uncorrupted version of the file #37294

Closed
wants to merge 1 commit into from
Closed

Updating upload.zip to be an uncorrupted version of the file #37294

wants to merge 1 commit into from

Conversation

gziskind
Copy link
Contributor

@gziskind gziskind commented Feb 9, 2021

I noticed that the file in question seemed to be corrupted and could not be unzipped. The original source of the file was copied from https://github.com/web-platform-tests/wpt/blob/master/FileAPI/filelist-section/support/upload.zip, so i replaced the version in the node repository with the original version of the file and confirmed that it was able to be unzipped.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 9, 2021
@targos
Copy link
Member

targos commented Feb 12, 2021

The file was copied using git node wpt so maybe there's a bug in that command for binary files. /cc @joyeecheung

@Trott
Copy link
Member

Trott commented Feb 18, 2021

The file was copied using git node wpt so maybe there's a bug in that command for binary files. /cc @joyeecheung

Seems like a bug (or I'm not understanding the command). rm -rf test/fixtures/wpt/FileAPI && git node wpt FileAPI reports that FileAPI is up to date. 🤔

Trott
Trott approved these changes Feb 18, 2021
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

In the meantime, I've confirmed that this file is correct. Puzzling that the test would pass with an invalid file, but maybe we don't run it or it's marked as skipped or whatever.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36201/

@targos
Copy link
Member

targos commented Feb 18, 2021

Puzzling that the test would pass with an invalid file, but maybe we don't run it or it's marked as skipped or whatever.

We probably don't run the test that uses this file. Currently we pull the FileAPI tests only to run the ones that test the Blob API.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 19, 2021
@joyeecheung
Copy link
Member

Seems like a bug (or I'm not understanding the command). rm -rf test/fixtures/wpt/FileAPI && git node wpt FileAPI reports that FileAPI is up to date. 🤔

You'd need to remove the hash in test/fixtures/wpt/versions.json - the command only checks the JSON to see if the data is up-to-date. I think the file was probably corrupted because it wasn't downloaded properly during the first update?

targos added a commit to targos/node-core-utils that referenced this pull request Feb 21, 2021
@targos
fix(wpt): download files as buffer instead of text
5479fb1 
There are binary files in the WPT repository and downloading them as
text corrupts them.

Refs: nodejs/node#37294
targos added a commit to targos/node-core-utils that referenced this pull request Feb 21, 2021
@targos
fix(wpt): download files as buffer instead of text
d97f83c 
There are binary files in the WPT repository and downloading them as
text corrupts them.

Refs: nodejs/node#37294
@targos targos mentioned this pull request Feb 21, 2021
Merged
@targos
Copy link
Member

targos commented Feb 21, 2021

PR to fix git node wpt: nodejs/node-core-utils#535

@targos
Copy link
Member

targos commented Feb 21, 2021

Landed in 574590d

targos pushed a commit that referenced this pull request Feb 21, 2021
@targos
test: update upload.zip to be uncorrupted
574590d 
PR-URL: #37294
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos closed this Feb 21, 2021
joyeecheung pushed a commit to nodejs/node-core-utils that referenced this pull request Feb 22, 2021
@targos
fix(wpt): download files as buffer instead of text (#535)
d6fad2a 
There are binary files in the WPT repository and downloading them as
text corrupts them.

Refs: nodejs/node#37294
@gziskind
Copy link
Contributor Author

Just for my knowledge, when would this get rolled into a release if its been merged into master?

@Trott
Copy link
Member

Trott commented Feb 24, 2021

Just for my knowledge, when would this get rolled into a release if its been merged into master?

Since it's a patch-fix, it will probably be in the next 15.x release, probably two weeks or so from now. Of course, since it's a test, it won't affect end users running node. But you'll see an entry in the changelog.

targos pushed a commit that referenced this pull request Feb 28, 2021
@targos
test: update upload.zip to be uncorrupted
4f41900 
PR-URL: #37294
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos mentioned this pull request Mar 2, 2021
Merged
@gziskind gziskind mentioned this pull request Sep 29, 2021
Merged
targos pushed a commit that referenced this pull request Sep 29, 2021
@targos
test: update upload.zip to be uncorrupted
b02a67f 
PR-URL: #37294
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
danielleadams pushed a commit that referenced this pull request Oct 12, 2021
@danielleadams
test: update upload.zip to be uncorrupted
92b678d 
PR-URL: #37294
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
danielleadams pushed a commit that referenced this pull request Oct 12, 2021
@danielleadams
test: update upload.zip to be uncorrupted
df401cd 
PR-URL: #37294
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@richardlau richardlau mentioned this pull request Nov 25, 2021
Merged
johnfrench3 pushed a commit to johnfrench3/core-utils-node that referenced this pull request Nov 2, 2022
fix(wpt): download files as buffer instead of text (#535)
444d229 
There are binary files in the WPT repository and downloading them as
text corrupts them.

Refs: nodejs/node#37294
renawolford6 added a commit to renawolford6/node-dev-build-core-utils that referenced this pull request Nov 10, 2022
@renawolford6
fix(wpt): download files as buffer instead of text (#535)
37647da 
There are binary files in the WPT repository and downloading them as
text corrupts them.

Refs: nodejs/node#37294
Developerarif2 pushed a commit to Developerarif2/node-core-utils that referenced this pull request Jan 27, 2023
@logictitans
fix(wpt): download files as buffer instead of text (#535)
a9f9939 
There are binary files in the WPT repository and downloading them as
text corrupts them.

Refs: nodejs/node#37294
gerkai added a commit to gerkai/node-core-utils-project-build that referenced this pull request Jan 27, 2023
@gerkai
fix(wpt): download files as buffer instead of text (#535)
50c02d5 
There are binary files in the WPT repository and downloading them as
text corrupts them.

Refs: nodejs/node#37294
patrickm68 added a commit to patrickm68/NodeJS-core-utils that referenced this pull request Sep 14, 2023
@patrickm68
fix(wpt): download files as buffer instead of text (#535)
38602a2 
There are binary files in the WPT repository and downloading them as
text corrupts them.

Refs: nodejs/node#37294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gziskind @targos @Trott @nodejs-github-bot @joyeecheung @lpinca @aduh95