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

added template for better error explanation to assert.ok error message #16829

Closed
wants to merge 5 commits into from

Conversation

@chrbergert
Copy link
Contributor

commented Nov 6, 2017

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
Affected core subsystem(s)
@@ -16,7 +16,7 @@ const args = [
{
const ret = spawnSync(process.execPath, args, { maxBuffer: 1 });

assert.ok(ret.error, 'maxBuffer should error');
assert.ok(ret.error, `instead of an error there is ${ret.error}` );

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 7, 2017

Member

Since ret is returned by spawnSync, ret.error can only be either an Error or undefined, so if it fails this would always be "instead of an error there is undefined", which does not seem very helpful... I think changing the message to child process does not error when maxBuffer is set to 1 would be easier to understand.

This comment has been minimized.

Copy link
@chrbergert

chrbergert Nov 7, 2017

Author Contributor

Good point, thank you

changed error message to a more speaking information about what happe…
…ned: "child process does not error when maxBuffer is set to 1"
@gireeshpunathil

This comment has been minimized.

@@ -16,7 +16,8 @@ const args = [
{
const ret = spawnSync(process.execPath, args, { maxBuffer: 1 });

assert.ok(ret.error, 'maxBuffer should error');
assert.ok(ret.error, 'child process does not error \
when maxBuffer is set to 1');

This comment has been minimized.

Copy link
@gireeshpunathil

gireeshpunathil Nov 7, 2017

Member

this your last commit introduced non-standard indentations. Please will you correct it? The previous commit was fair enough, dont know why you pushed again

This comment has been minimized.

Copy link
@chrbergert

chrbergert Nov 7, 2017

Author Contributor

thanks for the review. Sorry, that I have to change it again. I only wanted to fix the error that the linter was throwing, where i exceeded the maximum line length of 80.

@gireeshpunathil

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

Now that you pushed a commit while the CI was running (which would have picked up the older / newer commit depending on the timing of the push), the CI result is going to be inconclusive ping @Trott to confirm if that is the case.

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

Hi @chrbergert! Thanks for the PR and welcome. A few things:

  • Looking at this file and the task that was given out, I think this probably wasn't a good task. I'm really sorry about that. But let's keep going because there's stuff to do on this file anyway, and so we'll shift gears to that.

  • This fails linting because of the space before the close parentheses.

  • I'm not sure the new message is significantly better than the old message. I don't think the old message is as problematic as originally believed. Again, this is where I messed up curating tasks, so sorry about that!

So, a few things:

  • To check for lint errors, run make lint-js. It will take 20 or 30 seconds the first time, but should take only a second or two on subsequent runs because it caches results of unchanged files.

  • I'd say let's revert the error message change (or not if you or @joyeecheung think the new message is better--I personally think they're about the same so I'm fine with either one--one thing about the new error message is it )

  • If you do want to keep the new message, you can avoid the string concatenation like this:

assert.ok(ret.error,
          'child process does not error when maxBuffer is set to 1');

I think I'd prefer this:

assert.ok(ret.error, 'error expected when maxBuffer is set to 1');
  • Whether or not you change the contents of that assertion message, here are some changes to be made in the test, if you don't mind:

    • Remove the comment // This is actually not os.EOL?. Doesn't seem to be important/relevant.
    • Add a comment near the top (under require('common') explaining what the test does:
// This test checks that the maxBuffer option for child_process.spawnSync()
// works as expected.
@Trott
Copy link
Member

left a comment

requesting changes in comment above

- removed initial change for error message
- removed comment that wasn't relevant/important
- added comment that explains what the test does
@chrbergert

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2017

Hi together,

first of all, thanks for all the input and the warm welcome.
I now did the changes to the file like suggested:

  • removed initial change for error message
  • removed comment that wasn't relevant/important
  • added comment that explains what the test does

Before I ran lint-js locally and experimented a bit with it. Think I got the process for this, finally.

So, hope it goes through now and thanks again helping me doing my first contribution to this awesome project.

@lpinca

lpinca approved these changes Nov 8, 2017

@Trott

Trott approved these changes Nov 12, 2017

Copy link
Member

left a comment

(When landing, we'll need to be careful to make sure the commit message accurately reflects the change.)

@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

CI was good except for Raspberry Pi devices which have (had?) been having build problems. Let's try again: https://ci.nodejs.org/job/node-test-commit-arm-fanned/12541/

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

Landed in 21a7459.
Thanks for the contribution! 🎉

@Trott Trott closed this Nov 13, 2017

Trott added a commit to Trott/io.js that referenced this pull request Nov 13, 2017

test: refactor comments in test-child-process-spawnsync-maxbuf
* remove comment that isn't relevant/important
* add comment that explains what the test does

PR-URL: nodejs#16829
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas added a commit that referenced this pull request Nov 14, 2017

test: refactor comments in test-child-process-spawnsync-maxbuf
* remove comment that isn't relevant/important
* add comment that explains what the test does

PR-URL: #16829
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Nov 17, 2017

test: refactor comments in test-child-process-spawnsync-maxbuf
* remove comment that isn't relevant/important
* add comment that explains what the test does

PR-URL: #16829
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Nov 17, 2017

test: refactor comments in test-child-process-spawnsync-maxbuf
* remove comment that isn't relevant/important
* add comment that explains what the test does

PR-URL: #16829
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@gibfahn gibfahn referenced this pull request Nov 21, 2017

Merged

v8.9.2 proposal #17204

@MylesBorins MylesBorins referenced this pull request Nov 21, 2017

Merged

v6.12.1 proposal #17180

MylesBorins added a commit that referenced this pull request Nov 21, 2017

test: refactor comments in test-child-process-spawnsync-maxbuf
* remove comment that isn't relevant/important
* add comment that explains what the test does

PR-URL: #16829
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Nov 28, 2017

test: refactor comments in test-child-process-spawnsync-maxbuf
* remove comment that isn't relevant/important
* add comment that explains what the test does

PR-URL: #16829
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

test: refactor comments in test-child-process-spawnsync-maxbuf
* remove comment that isn't relevant/important
* add comment that explains what the test does

PR-URL: nodejs#16829
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@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
You can’t perform that action at this time.