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.

Sign up for GitHub

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

Jump to bottom

test: fix error when foo in path to git clone #14506

Closed
wants to merge 5 commits into from
Closed

test: fix error when foo in path to git clone #14506

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0bd6ac8
"test: fix error when foo in path to git clone
SwimmingSage Jul 26, 2017
8d7445c
"test assert: edit to regex error when foo in path to git clone
SwimmingSage Jul 26, 2017
8bcca8f
"test assert: edit to test case when foo is in path to git clone
SwimmingSage Jul 26, 2017
ed89e4b
"test assert: update to prior commit, more specific regex
SwimmingSage Jul 26, 2017
441a7b4
"test assert: update to previous commit of regex
SwimmingSage Jul 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/parallel/test-assert-fail.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,5 @@ common.expectsError(() => {
// The stackFrameFunction should exclude the foo frame
assert.throws(
function foo() { assert.fail('first', 'second', 'message', '!==', foo); },
(err) => !/foo/m.test(err.stack)
(err) => !/foo(?!\/)/m.test(err.stack)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@SwimmingSage SwimmingSage Jul 26, 2017 •
edited by Trott
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);