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: move test-fs-largefile to pummel #14338

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 18, 2017

test-fs-largefile was disabled. It was fixed in bbf74fb but left in
disabled because it generates a 5Gb file. However, gibfahn had the
sensible suggestion of moving it to the pummel directory. Which is what
this change does.

In pummel, lint rules are applied, so this does necessitate changing a
pair of var declarations to const.

Refs: bbf74fb

h/t @gibfahn

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

test fs

test-fs-largefile was disabled. It was fixed in bbf74fb but left in
disabled because it generates a 5Gb file. However, gibfahn had the
sensible suggestion of moving it to the pummel directory. Which is what
this change does.

In pummel, lint rules are applied, so this does necessitate changing a
pair of `var` declarations to `const`.

Refs: nodejs@bbf74fb
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 18, 2017
@Trott Trott added the fs Issues and PRs related to the fs subsystem / file system. label Jul 18, 2017
@Trott Trott mentioned this pull request Jul 18, 2017
2 tasks
@vsemozhetbyt
Copy link
Contributor

Should we check if the filesystem is FAT32 on Windows? FAT32 cannot hold a 5 GB file.

@gibfahn
Copy link
Member

gibfahn commented Jul 18, 2017

Should we check if the filesystem is FAT32 on Windows? FAT32 cannot hold a 5 GB file.

We should, but I don't think it's a blocker as we don't guarantee that pummel tests will run on all machines.

So practically speaking, if you can suggest a line for @Trott to add that'd be great!

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 18, 2017

@Trott @gibfahn I can think up something like this, but there may be more efficient ways.

if (common.isWindows) {
  const { execFileSync } = require('child_process');
  const driveLetter = common.tmpDir[0];
  const fileSystem = execFileSync(
    'fsutil', ['fsinfo', 'volumeinfo', `${driveLetter}:`], { encoding: 'utf8' }
  ).match(/^File System Name.+\b(\w+)$/m)[1];

  if (fileSystem.startsWith('FAT'))
    common.skip('FAT* file systems do not support such large files');
}

cc @nodejs/platform-windows

@gibfahn
Copy link
Member

gibfahn commented Jul 18, 2017

@vsemozhetbyt that's where I feel that maybe having that as a separate PR for that change might be better, said follow-on PR could also include the other checks @Trott mentioned:

(definitely disk space, likely memory, maybe check that it's a 64-bit OS).

@refack
Copy link
Contributor

refack commented Jul 18, 2017

FYI The root of FAT drive does not have an inode number, but it's true for exFAT as well:
So a line could be:

const isFAT = common.isWindows &&
  require('fs').statSync(common.tmpDir.slice(0,3)).ino === 0;

NTFS

> fs.statSync('c:\\')
Stats {
  dev: 3228880019,
  mode: 16822,
  nlink: 1,
  uid: 0,
  gid: 0,
  rdev: 0,
  blksize: undefined,
  ino: 1407374883553285,
  size: 0,
  blocks: undefined,
  atimeMs: 1500174278591.7847,
  mtimeMs: 1500174278591.7847,
  ctimeMs: 1500174278591.7847,
  birthtimeMs: 1468649064614.482,
  atime: 2017-07-16T03:04:38.592Z,
  mtime: 2017-07-16T03:04:38.592Z,
  ctime: 2017-07-16T03:04:38.592Z,
  birthtime: 2016-07-16T06:04:24.614Z }

FAT32

> fs.statSync('e:\\')
Stats {
  dev: 1244641795,
  mode: 16822,
  nlink: 1,
  uid: 0,
  gid: 0,
  rdev: 0,
  blksize: undefined,
  ino: 0,
  size: 0,
  blocks: undefined,
  atimeMs: 315547200000,
  mtimeMs: 315547200000,
  ctimeMs: -921101621044.8384,
  birthtimeMs: 315547200000,
  atime: 1980-01-01T04:00:00.000Z,
  mtime: 1980-01-01T04:00:00.000Z,
  ctime: 1940-10-24T02:26:18.956Z,
  birthtime: 1980-01-01T04:00:00.000Z }

exFAT

> fs.statSync('t:\\')
Stats {
  dev: 1587103641,
  mode: 16822,
  nlink: 1,
  uid: 0,
  gid: 0,
  rdev: 0,
  blksize: undefined,
  ino: 0,
  size: 0,
  blocks: undefined,
  atimeMs: 315547200000,
  mtimeMs: 315547200000,
  ctimeMs: -921101621044.8384,
  birthtimeMs: 315547200000,
  atime: 1980-01-01T04:00:00.000Z,
  mtime: 1980-01-01T04:00:00.000Z,
  ctime: 1940-10-24T02:26:18.956Z,
  birthtime: 1980-01-01T04:00:00.000Z }

@vsemozhetbyt
Copy link
Contributor

@refack Does not this check disable the test for exFAT which has not this file size limit?

@refack
Copy link
Contributor

refack commented Jul 18, 2017

@refack Does not this check disable the test for exFAT which has not this file size limit?

Yes, it is too strict.

@Trott
Copy link
Member Author

Trott commented Jul 19, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9258/ (shouldn't matter because pummel tests don't get run, so only linting is relevant, BUT YOU CAN'T BE TOO CAREFUL... point is, if there's a backlog, feel free to cancel anything but the linter)

@gibfahn
Copy link
Member

gibfahn commented Jul 20, 2017

CI is green, landing.

@gibfahn
Copy link
Member

gibfahn commented Jul 20, 2017

Landed in 9d9c9c1

@gibfahn gibfahn closed this Jul 20, 2017
gibfahn pushed a commit that referenced this pull request Jul 20, 2017
test-fs-largefile was disabled. It was fixed in bbf74fb but left in
disabled because it generates a 5Gb file. However, gibfahn had the
sensible suggestion of moving it to the pummel directory. Which is what
this change does.

In pummel, lint rules are applied, so this does necessitate changing a
pair of `var` declarations to `const`.

PR-URL: #14338
Refs: bbf74fb
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 22, 2017
test-fs-largefile was disabled. It was fixed in bbf74fb but left in
disabled because it generates a 5Gb file. However, gibfahn had the
sensible suggestion of moving it to the pummel directory. Which is what
this change does.

In pummel, lint rules are applied, so this does necessitate changing a
pair of `var` declarations to `const`.

PR-URL: #14338
Refs: bbf74fb
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
test-fs-largefile was disabled. It was fixed in bbf74fb but left in
disabled because it generates a 5Gb file. However, gibfahn had the
sensible suggestion of moving it to the pummel directory. Which is what
this change does.

In pummel, lint rules are applied, so this does necessitate changing a
pair of `var` declarations to `const`.

PR-URL: #14338
Refs: bbf74fb
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jul 24, 2017
@addaleax addaleax mentioned this pull request Aug 2, 2017
@Trott Trott deleted the move-to-pummel branch January 13, 2022 22:46
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.

None yet

9 participants