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: modernize JS in some test-fs-*.js #23031

Closed
wants to merge 13 commits into from

Conversation

@jy95
Copy link
Contributor

commented Sep 22, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Description

Replaced several variable initialization with spread / forEach / Object.assign(...Object.entries(obj).map()) .

@Trott
Copy link
Member

left a comment

Hi, @jy95! Welcome and thanks for the pull request.

Most of these changes are 👍 but I'm not sure about the .map() usages for assigning constants because:

  • It makes the code less readable/maintainable. (Example: The use of .map() in test-fs-access.js makes that code more difficult to understand and maintain for no discernible benefit.)

  • It makes it more difficult to introduce block scoping of these constants for individual test cases. (Example: test-fs-append-file-sync.js changes. I'd much rather see each variable block scoped to just the test case that needs it but using .map() to assign them all at the outset prevents that.)

Would you consider removing those or at least moving them to a separate pull request so that they can be considered/discussed separately and the more straightforward changes (like introducing the ternary in test-fs-open-mode-mask.js to make it possible to make mode a constant) can be merged separately?

@jy95

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2018

I removed them : I only keep the ternary way because I think it is more clear in that way ^^

@@ -13,10 +13,12 @@ const { internalBinding } = require('internal/test/binding');
const { UV_ENOENT } = internalBinding('uv');

const tmpdir = require('../common/tmpdir');

This comment has been minimized.

Copy link
@tniessen

tniessen Sep 23, 2018

Member

Nit: Unrelated whitespace change.

This comment has been minimized.

Copy link
@jy95

jy95 Sep 23, 2018

Author Contributor

Oups : fixed

const doesNotExist = path.join(tmpdir.path, '__this_should_not_exist');
const readOnlyFile = path.join(tmpdir.path, 'read_only_file');
const readWriteFile = path.join(tmpdir.path, 'read_write_file');


This comment has been minimized.

Copy link
@tniessen

tniessen Sep 23, 2018

Member

Ditto.

This comment has been minimized.

Copy link
@jy95

jy95 Sep 23, 2018

Author Contributor

Oups : fixed

@@ -7,13 +7,8 @@ const assert = require('assert');
const path = require('path');
const fs = require('fs');

let mode;
const mode = (common.isWindows) ? 0o444 : 0o644;

This comment has been minimized.

Copy link
@tniessen

tniessen Sep 23, 2018

Member

Nit: Parentheses unnecessary.

This comment has been minimized.

Copy link
@jy95

jy95 Sep 23, 2018

Author Contributor

I know that they are not necessary for simple case (like this one). ESLint didn't remove them

@Trott

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

Thanks! One other thing: Can you also leave the fiilename2 etc. declarations where they are in that one file? By moving them to the top,it makes block-scoping less obvious.

const O_TRUNC = fs.constants.O_TRUNC || 0;
const O_WRONLY = fs.constants.O_WRONLY || 0;
// 0 if not found in fs.constants
const { O_APPEND = 0, O_CREAT = 0, O_EXCL = 0, O_RDONLY = 0, O_RDWR = 0,

This comment has been minimized.

Copy link
@devsnek

devsnek Sep 23, 2018

Member

Can you keep these each on separate lines?

const {
  O_APPEND = 0,
  O_CREAT = 0,
  ...
} = fs.constants;

This comment has been minimized.

Copy link
@jy95

jy95 Sep 23, 2018

Author Contributor

Ok for readability : I will change

bigintStats.isSymbolicLink(),
numStats.isSymbolicLink()
);
['isBlockDevice', 'isCharacterDevice', 'isDirectory', 'isFIFO', 'isFile',

This comment has been minimized.

Copy link
@devsnek

devsnek Sep 23, 2018

Member

separate lines

This comment has been minimized.

Copy link
@jy95

jy95 Sep 23, 2018

Author Contributor

You mean one line for each item of the array ? Sure if you want

assert.strictEqual(typeof UV_FS_COPYFILE_FICLONE, 'number');
assert.strictEqual(typeof UV_FS_COPYFILE_FICLONE_FORCE, 'number');
[COPYFILE_EXCL, COPYFILE_FICLONE, COPYFILE_FICLONE_FORCE, UV_FS_COPYFILE_EXCL,
UV_FS_COPYFILE_FICLONE, UV_FS_COPYFILE_FICLONE_FORCE]

This comment has been minimized.

Copy link
@devsnek

devsnek Sep 23, 2018

Member

separate lines

This comment has been minimized.

Copy link
@jy95

jy95 Sep 23, 2018

Author Contributor

Same that above : I will change

@jy95

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2018

@Trott You mean : put them in their original place ? Sorry but I don't see the problem with the current block-scoping ... I will still change it as someone agreed with you :)

Hope I succeed my first commit squashing after these changes ^^

@Trott

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

@jy95 What I mean is that if we want to add block scoping for the individual test cases inside the file (and we do), then the declarations will have to be in separate blocks. The declarations will have to be where they are now, basically. So we'll just end up moving them back.

@Trott

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

(By the way, if you want to block scope each of the test cases in that file, go for it.)

@jy95

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2018

I don't think it will be useful for this file ...

@BridgeAR
Copy link
Member

left a comment

I am not convinced about these changes as they decrease the readability in an error case when changing the individual assertions to an array.

Each assertion will currently show the line in the stack trace but when changing it to an array, we have to add extra information to the output to know what entry actually failed.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

@jy95 thanks a lot for the PR! There is definitely a lot of code that needs cleanup, I am just not sure that all of these changes will actually lead to the better. I do like the destructuring though :)

@jy95

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2018

It is also possible to add a custom message using the forEach parameter index ;

[fs.F_OK, fs.R_OK, fs.W_OK, fs.X_OK].forEach(
  (cons) => assert.strictEqual(typeof cons, 'number', 
                   `The foreach test for fs constants for index: ${index}`)
);

or using Object.entries :

Object.entries(
    { 'fs.F_OK'  : fs.F_OK , 'fs.R_OK': fs.R_OK, 'fs.W_OK': fs.W_OK, 'fs.X_OK': fs.X_OK  }
).forEach(
   ([varName,val]) => assert.strictEqual(typeof val, 'number', `Test failed for fs.${varName}`)
)

Notice : There is maybe a way to create the object automatically if there exist a function to get javascript variable name ...

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

@jy95 I am aware that it's possible to add a message property and if the test would have done that originally, I would have signed off on it (in case it would not only contain the index but also the actual difference of the assertion). Changing it now does not seem to improve anything as the actual code is more difficult to read and not a lot of code lines are reduced this way.

@jy95

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

For the moment, I only verified +/- 20 test files but what I saw, generic function(s) could improve at least readability / maintenance. For example , using a structure like this (first draft : surely there are things that must evolue ) :

{
   // the name you want to see in console log if there is assert error(s) 
   testCaseName: 'Testing Fs constants'
   // The default assert fct that would be invoked
   defaultAssertFct: assert.strictEqual,
   // defaultMessage (hidden param) : it will be an AssertionError with default message
   // '${testCaseName} - The test N°${index} failed' , other parameters will be the same one
   tests: [
       // actual and expected are required , you can override the default message
       {actual : typeof fs.F_OK, expected: 'number', message: 'F_OK failed'},
       // possibility to change assert function (optional, default to defaultAssertFct)
       {actual : 1, expected: 2, assertFct: assert.notDeepStrictEqual},
       // ... other idea can improve this basic design
   ] 
}

Of couse, it is completely up to you to choose how you initialize the tests array following the circumstances ^^ For example , I prefer to write on the readability field :

{
  testCaseName: 'Testing Fs constants',
  defaultAssertFct: assert.strictEqual,
  tests: [fs.F_OK, fs.R_OK, fs.W_OK, fs.X_OK].map( 
     (cons) => {actual: typeof cons, expected: 'number' } 
  )
}

instead of

[fs.F_OK, fs.R_OK, fs.W_OK, fs.X_OK].forEach( 
      (cons) => assert.strictEqual(typeof cons, 'number') 
);
@tniessen

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

The original version (four separate assertions) seems much more readable than both of these. I'd prefer to use for .... of if we want to iterate, but that's personal preference.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

I agree with @tniessen and I think it would be best to just revert the assertion changes.

@jy95

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

Maybe I will ask review about the feature request (last message) I would like to add inside the assert into a another issue .
After review of your opinion , I will revert the assertion changes in test/parallel/test-fs-copyfile.js and test/parallel/test-fs-access.js.

For the one in test/parallel/test-fs-stat-bigint.js , I disagree about the original : it is quite unreadable in my opinion and from what I saw in the docs , these getters are here from a long time ( v0.1.10) without parameters ...

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

For the one in test/parallel/test-fs-stat-bigint.js , I disagree about the original : it is quite unreadable in my opinion and from what I saw in the docs , these getters are here from a long time ( v0.1.10) without parameters ...

I agree with @tniessen and @BridgeAR that the current version is a bit better from a DAMP-not-DRY perspective.

More concretely, the refactor makes the assertions less useful. In the current test, if an assertion fails, you know exactly why because it's right there in the line of code that failed:

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

false !== true

    at verifyStats (/Users/trott/io.js/test/parallel/test-fs-stat-bigint.js:53:14)

Line 53-56 is:

     assert.strictEqual(
        bigintStats.isSocket(),
        numStats.isSocket()
      );

If we instead go with what's in this PR, we won't know that it's the isSocket() check that's failing. We'll just know that one of the properties in the array. We won't know which one.

That can be solved by adding a third argument to assert.strictEqual() that indicates which property is being tested.

I'd greatly prefer those changes be moved to their on PR and we land the improvements here in the other two files.

@Trott Trott dismissed their stale review Nov 22, 2018

map() change was removed

@jy95

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

Ok : feel free to change it if you prefer that way ...

@Trott
Trott approved these changes Nov 28, 2018
@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

@Trott Trott added the author ready label Dec 4, 2018

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

Landed in 841caef.

Thanks for the contribution! 🎉

Trott added a commit to Trott/io.js that referenced this pull request Dec 4, 2018
test: use ES2017 syntax in test-fs-open-*
Update test-fs-open-flags to take advantage of destructuring and default
values.

Update test-fs-open-mode-mask to use a ternary to make use of a
constant possible.

PR-URL: nodejs#23031
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@Trott Trott closed this Dec 4, 2018

BridgeAR added a commit that referenced this pull request Dec 5, 2018
test: use ES2017 syntax in test-fs-open-*
Update test-fs-open-flags to take advantage of destructuring and default
values.

Update test-fs-open-mode-mask to use a ternary to make use of a
constant possible.

PR-URL: #23031
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Dec 5, 2018
test: use ES2017 syntax in test-fs-open-*
Update test-fs-open-flags to take advantage of destructuring and default
values.

Update test-fs-open-mode-mask to use a ternary to make use of a
constant possible.

PR-URL: #23031
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR referenced this pull request Dec 5, 2018
4 of 4 tasks complete
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
test: use ES2017 syntax in test-fs-open-*
Update test-fs-open-flags to take advantage of destructuring and default
values.

Update test-fs-open-mode-mask to use a ternary to make use of a
constant possible.

PR-URL: nodejs#23031
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs added a commit that referenced this pull request Feb 12, 2019
test: use ES2017 syntax in test-fs-open-*
Update test-fs-open-flags to take advantage of destructuring and default
values.

Update test-fs-open-mode-mask to use a ternary to make use of a
constant possible.

PR-URL: #23031
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs referenced this pull request Feb 12, 2019
BethGriggs added a commit that referenced this pull request Feb 20, 2019
test: use ES2017 syntax in test-fs-open-*
Update test-fs-open-flags to take advantage of destructuring and default
values.

Update test-fs-open-mode-mask to use a ternary to make use of a
constant possible.

PR-URL: #23031
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg added a commit that referenced this pull request Feb 28, 2019
test: use ES2017 syntax in test-fs-open-*
Update test-fs-open-flags to take advantage of destructuring and default
values.

Update test-fs-open-mode-mask to use a ternary to make use of a
constant possible.

PR-URL: #23031
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.