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

test: fix disabled test-fs-largefile #13147

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented May 22, 2017

Add common.refreshTmpDir() before using the tmp directory.

fs.writeSync no longer requires an integer for the position argument, so
change test from assert.throws() to assert.doesNotThrow(). The test
now passes.

Because it involves a 5 GB file, we're not going to activate the test,
although that is possible if we add checking for appropriate available
resources (definitely disk space, likely memory, maybe check that it's a
64-bit OS).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test fs

Add `common.refreshTmpDir()` before using the tmp directory.

fs.writeSync no longer requires an integer for the position argument, so
change test from `assert.throws()` to `assert.doesNotThrow()`. The test
now passes.

Because it involves a 5 GB file, we're not going to activate the test,
although that is possible if we add checking for appropriate available
resources (definitely disk space, likely memory, maybe check that it's a
64-bit OS).
@Trott Trott added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels May 22, 2017
@vsemozhetbyt
Copy link
Contributor

Also, consider file system restrictions (FAT32 cannot have a 5 GB file etc).

@Trott
Copy link
Member Author

Trott commented May 24, 2017

@Trott
Copy link
Member Author

Trott commented May 24, 2017

Sole CI failure is an unrelated build issue.

Trott added a commit to Trott/io.js that referenced this pull request May 24, 2017
Add `common.refreshTmpDir()` before using the tmp directory.

fs.writeSync no longer requires an integer for the position argument, so
change test from `assert.throws()` to `assert.doesNotThrow()`. The test
now passes.

Because it involves a 5 GB file, we're not going to activate the test,
although that is possible if we add checking for appropriate available
resources (definitely disk space, likely memory, maybe check that it's a
64-bit OS).

PR-URL: nodejs#13147
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented May 24, 2017

Landed in bbf74fb

@Trott Trott closed this May 24, 2017
jasnell pushed a commit that referenced this pull request May 25, 2017
Add `common.refreshTmpDir()` before using the tmp directory.

fs.writeSync no longer requires an integer for the position argument, so
change test from `assert.throws()` to `assert.doesNotThrow()`. The test
now passes.

Because it involves a 5 GB file, we're not going to activate the test,
although that is possible if we add checking for appropriate available
resources (definitely disk space, likely memory, maybe check that it's a
64-bit OS).

PR-URL: #13147
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request May 28, 2017
Add `common.refreshTmpDir()` before using the tmp directory.

fs.writeSync no longer requires an integer for the position argument, so
change test from `assert.throws()` to `assert.doesNotThrow()`. The test
now passes.

Because it involves a 5 GB file, we're not going to activate the test,
although that is possible if we add checking for appropriate available
resources (definitely disk space, likely memory, maybe check that it's a
64-bit OS).

PR-URL: #13147
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Contributor

should this land on v6.x?

@Trott
Copy link
Member Author

Trott commented Jul 18, 2017

should this land on v6.x?

@MylesBorins I would say "no" and have added dont-land labels for 6.x and 4.x.

@gibfahn
Copy link
Member

gibfahn commented Jul 18, 2017

@Trott maybe we could add this test to pummel?

@Trott
Copy link
Member Author

Trott commented Jul 18, 2017

@gibfahn Good suggestion. #14338

@Trott Trott deleted the large-file branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants