test: fix error when foo in path to git clone by SwimmingSage · Pull Request #14506 · nodejs/node

SwimmingSage · 2017-07-26T21:53:28Z

I fixed an error that occurred in the test case of the file
test/parallel/test-assert-fail.js when foo was in the path to
the git clone. This occurred due to a regex that looked only for the
word foo, and so it was updated to not look for foo/, but only
foo. This way it won't go off from foo being in the path to the
git clone.

What this part of the test verifies is that the last argument
to assert.fail in
function foo() { assert.fail('first', 'second', 'message', '!==', foo); },
(the , foo one) is not ignored. If it is ignored an error would appear
stating at foo near the top of the error message, thus if the path
contains foo it will not trigger the error message

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

I fixed an error that occured in the test case of the file test/parallel/test-assert-fail.js when foo was in the path to the git clone. This occured due to a regex that looked only for the word foo, and so it was updated to not look for foo/, but only foo. This way it won't go off from foo being in the path to the git clone"

addaleax · 2017-07-26T21:57:34Z

test/parallel/test-assert-fail.js

 assert.throws(
  function foo() { assert.fail('first', 'second', 'message', '!==', foo); },
-  (err) => !/foo/m.test(err.stack)
+  (err) => !/foo(?!\/)/m.test(err.stack)


This is enough to cover /path/with/foo/node, but it wouldn’t catch e.g. /path/with/foo-bar/node (or, for that matter, Windows paths that use \), right?

It still looks good to me as it’s strictly an improvement, but maybe there are better options.
It might be enough if the regex checks for (whitespace)at foo , because that’s how the function names show up?

Ok, I was unsure of what this error would actually look like, as the error message I received was in this block:

AssertionError [ERR_ASSERTION]: message at tryBlock (assert.js:595:5) at innerThrows (assert.js:614:18) at Function.throws (assert.js:641:3) at Object.<anonymous> (/Users/SwimmingSage/githubProjects/**foo**/node/test/parallel/test-assert-fail.js:68:8) at Module._compile (module.js:573:30) at Object.Module._extensions..js (module.js:584:10) at Module.load (module.js:507:32) at tryModuleLoad (module.js:470:12) at Function.Module._load (module.js:462:3) at Function.Module.runMain (module.js:609:10)

So I simply tried to catch for that case, would you happen to know by chance where in that block foo would be mentioned if an actual error went off? I didn't want to be too specific as I was worried about interfering when there actually should be an error. But I definitely agree that this can be made more specific

Oh sorry, I just saw that you did in fact mention that, my apologies, I will edit it for:

(whitespace)at foo

Thank your for your feedback!

So I simply tried to catch for that case, would you happen to know by chance where in that block foo would be mentioned if an actual error went off?

What this part of the test verifies is that the last argument to assert.fail in function foo() { assert.fail('first', 'second', 'message', '!==', foo); }, (the , foo one) is not ignored. If you remove that bit, you’ll see something like an additional at foo line near the top.

(Since this is obviously not obvious, you could add a comment here that explains what the test does as well?)

Ok, will do, thanks!

BTW, it seems that /m flag is redundant here, as it is useful only if ^$ symbols are used.

Ok, I'll get rid of that then, thank you for the feedback!

"

vsemozhetbyt · 2017-07-26T22:21:25Z

test/parallel/test-assert-fail.js

 assert.throws(
  function foo() { assert.fail('first', 'second', 'message', '!==', foo); },
-  (err) => !/foo/m.test(err.stack)
+  (err) => !/\sat\sfoo/.test(err.stack)


Maybe /\sat\sfoo\b/ to be extra careful?

Yeah, I can do that, thank you for the feedback again

"

addaleax

Thanks!
One tiny suggestion for even more strictness: start with ^\s* 😄

"

SwimmingSage · 2017-07-26T22:30:13Z

@addaleax Done, thank you for the suggestion!

vsemozhetbyt · 2017-07-26T22:32:59Z

@addaleax is this ^ OK without the /m flag? Should we check only the first stack line?

addaleax · 2017-07-26T22:35:07Z

@vsemozhetbyt Aaaah sorry yes, it should get the /m flag again.

"

vsemozhetbyt · 2017-07-26T22:39:27Z

@SwimmingSage Thank you for the patience and sorry for all the nudging. Can you add the /m flag again?

SwimmingSage · 2017-07-26T22:40:21Z

@vsemozhetbyt Yeah, I just did, and no worries, this is my first time helping out on an open source project and I greatly appreciate the feedback

vsemozhetbyt · 2017-07-26T22:40:59Z

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

vsemozhetbyt · 2017-07-27T01:31:36Z

Two windows failures due to inspector/test-inspector-stop-profile-after-done (seems unrelated?)

Trott · 2017-07-27T05:29:12Z

Two windows failures due to inspector/test-inspector-stop-profile-after-done (seems unrelated?)

@vsemozhetbyt Yes, unfortunately that test seems to be in perma-failure mode right now. #14507

addaleax · 2017-07-30T11:07:08Z

Landed in 1424e9e, thanks for the contribution!

I fixed an error that occured in the test case of the file test/parallel/test-assert-fail.js when foo was in the path to the git clone. This occured due to a regex that looked only for the word foo, and so it was updated to not look for foo/, but only foo. This way it won't go off from foo being in the path to the git clone PR-URL: #14506 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 26, 2017

addaleax reviewed Jul 26, 2017

View reviewed changes

vsemozhetbyt added the assert Issues and PRs related to the assert subsystem. label Jul 26, 2017

"test assert: edit to regex error when foo in path to git clone

8d7445c

"

vsemozhetbyt approved these changes Jul 26, 2017

View reviewed changes

vsemozhetbyt reviewed Jul 26, 2017

View reviewed changes

"test assert: edit to test case when foo is in path to git clone

8bcca8f

"

addaleax approved these changes Jul 26, 2017

View reviewed changes

"test assert: update to prior commit, more specific regex

ed89e4b

"

tniessen approved these changes Jul 26, 2017

View reviewed changes

"test assert: update to previous commit of regex

441a7b4

"

vsemozhetbyt mentioned this pull request Jul 27, 2017

inspector: send messages after the Node is done #14463

Merged

3 tasks

lpinca approved these changes Jul 27, 2017

View reviewed changes

addaleax closed this Jul 30, 2017

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

MylesBorins added the dont-land-on-v6.x label Aug 16, 2017

Uh oh!

Conversation

SwimmingSage commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SwimmingSage Jul 26, 2017 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

SwimmingSage commented Jul 26, 2017

Uh oh!

vsemozhetbyt commented Jul 26, 2017

Uh oh!

addaleax commented Jul 26, 2017

Uh oh!

vsemozhetbyt commented Jul 26, 2017

Uh oh!

SwimmingSage commented Jul 26, 2017

Uh oh!

vsemozhetbyt commented Jul 26, 2017

Uh oh!

vsemozhetbyt commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 27, 2017

Uh oh!

addaleax commented Jul 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

SwimmingSage commented Jul 26, 2017 •

edited

Loading

SwimmingSage Jul 26, 2017 •

edited by Trott

Loading

vsemozhetbyt commented Jul 27, 2017 •

edited

Loading