test: add regex to assert in test_cyclic_link_protection #11498

clarenced · 2017-02-22T09:01:58Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

jasnell · 2017-02-22T15:40:39Z

test/parallel/test-fs-realpath.js

@@ -201,9 +201,11 @@ function test_cyclic_link_protection(callback) {
    fs.symlinkSync(t[1], t[0], 'dir');
    unlink.push(t[0]);
  });
-  assert.throws(function() { fs.realpathSync(entry); });
+  assert.throws(function() { fs.realpathSync(entry); }, /^Error: ELOOP/);


You can use common.expectError(undefined, Error, 'ELOOP)` here I believe. (Or some variation on that)

Actually... common.expectsError('ELOOP', Error') should work.

One downside to common.expectsError() is that it increases the abstractions people have to learn about in order to work with even simple tests. Whereas before, all they had to know about is assert.throws(), now they have to also know about common.expectsError(). This is very different (IMO) from common.mustCall() in that the latter has an intuitively understandable name, accepts arguments as one would expect, and (in most cases) doesn't actually require reading documentation or code to figure out what it's doing. With common.expectsError(), there's a fair bit of magic. Like, it's not obvious what the first argument would be. Or the second. Or the third. You just have to know.

For that reason, I'm inclined to accept (encourage even) people submitting first PRs that add a regexp as a second argument to assert.throws() calls that previously only had a single argument. Changing the regexp to common.expectsError() can be their second PR or a subsequent PR from someone else.

One thing that might help is if the arguments accepted by common.expectsError() were a single settings object. Someone coming across this has a hope of understanding what's going on without reading source or docs:

const validatorFunction = common.expectsError({code: 'ELOOP', type: Error, message: 'looped doodad found'});

This, by comparison, is harder to grok:

const validatorFunction = common.expectsError('ELOOP', Error, 'looped doodad found');

And this is especially wat-inducing:

common.expectsError(undefined, undefined, 'looped doodad found');

It's likely that only people who work with tests frequently can be expected to remember the three arguments and their order. By comparison, remembering that the error code is code and the message is message might be manageable.

(I'm spinning up a PR for the proposed API change right now.)

Another thing that might help is if the arguments accepted by common.expectsError() was a single settings object.

Yeah that looks way easier to understand (as someone who hasn't yet gotten round to reading the source or docs for common.expectsError()).

jasnell · 2017-02-22T15:42:03Z

test/parallel/test-fs-realpath.js

@@ -201,9 +201,11 @@ function test_cyclic_link_protection(callback) {
    fs.symlinkSync(t[1], t[0], 'dir');
    unlink.push(t[0]);
  });
-  assert.throws(function() { fs.realpathSync(entry); });
+  assert.throws(function() { fs.realpathSync(entry); }, /^Error: ELOOP/);
  asynctest(fs.realpath, [entry], callback, function(err, result) {


It would likely be worthwhile to wrap the function(err, result) {} block in a common.mustCall() throughout as an extra check

jasnell · 2017-02-27T21:57:12Z

@clarenced ... can you rebase this on the current master to avoid the extraneous commits?

clarenced · 2017-02-28T02:00:26Z

@jasnell Sorry I have made the same mistake again, I forgot to rebase on origin/master. Is there a way to correct that?
`

Trott · 2017-02-28T07:25:45Z

@clarenced I was able to fix it by rebasing against upstream/master, resolving a merge conflict, and force-pushing to your branch. You're all set for the moment, I think.

(In the future, it would be a good idea to not use your master branch for development but to instead create a separate branch where development occurs.)

Trott · 2017-02-28T07:28:24Z

test/parallel/test-fs-realpath.js

+  }, common.expectsError({ code: 'ELOOP', type: Error }));
+  asynctest(
+    fs.realpath, [entry], callback, common.mustCall(function(err, result) {
+      assert.ok(err.path === entry);


Can you use assert.strictEqual() here and in the line below:

assert.strictEqual(err.path, entry); assert.strictEqual(result, undefined);

That will provide better error messages should the assertion fire.

@Trott Hi, I have made the modification as requested in another branch called test_11498. #11622

Trott · 2017-02-28T08:03:57Z

@clarenced I should have probably mentioned that you'll want to update from the force-pushed branch in a way that won't create a merge commit on your branch. Many ways to do it, here's one:

git checkout master
git reset --hard ec4440a
git pull

(That ec440a is the most recent commit that was on your branch (prior to your own commits) before I rebased.)

Trott · 2017-03-01T00:37:32Z

Closing in favor of #11622

hiroppy added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Feb 22, 2017

jasnell requested changes Feb 22, 2017

View reviewed changes

clarenced added 6 commits February 27, 2017 23:18

test: add regex to match error message

d062327

test: lint error correction

38ad8d7

test: add regex to match error message

3dd9731

test: lint error correction

77fb1b3

test: test assertions improvements

4370eb5

test: update the call of the function common.expectsError

1130abd

Trott reviewed Feb 28, 2017

View reviewed changes

Trott closed this Mar 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add regex to assert in test_cyclic_link_protection #11498

test: add regex to assert in test_cyclic_link_protection #11498

clarenced commented Feb 22, 2017

jasnell Feb 22, 2017

jasnell Feb 22, 2017

Trott Feb 22, 2017 •

edited

Loading

Trott Feb 22, 2017

gibfahn Feb 22, 2017

Trott Feb 22, 2017

jasnell Feb 22, 2017

jasnell commented Feb 27, 2017

clarenced commented Feb 28, 2017 •

edited

Loading

Trott commented Feb 28, 2017

Trott Feb 28, 2017

clarenced Feb 28, 2017

Trott commented Feb 28, 2017

Trott commented Mar 1, 2017

test: add regex to assert in test_cyclic_link_protection #11498

test: add regex to assert in test_cyclic_link_protection #11498

Conversation

clarenced commented Feb 22, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Feb 27, 2017

clarenced commented Feb 28, 2017 • edited Loading

Trott commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Feb 28, 2017

Trott commented Mar 1, 2017

Trott Feb 22, 2017 •

edited

Loading

clarenced commented Feb 28, 2017 •

edited

Loading