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

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)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2017
@gireeshpunathil gireeshpunathil added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2017
@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Nov 6, 2017
@@ -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}` );
Copy link
Member

@joyeecheung joyeecheung Nov 7, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thank you

…ned: "child process does not error when maxBuffer is set to 1"
@gireeshpunathil
Copy link
Member

@@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

Trott 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.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

requesting changes in comment above

- removed comment that wasn't relevant/important
- added comment that explains what the test does
@chrbergert
Copy link
Contributor Author

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.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Nov 12, 2017

@Trott
Copy link
Member

Trott 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
Copy link
Member

Trott commented Nov 13, 2017

Landed in 21a7459.
Thanks for the contribution! 🎉

@Trott Trott closed this Nov 13, 2017
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 13, 2017
* 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 pushed a commit that referenced this pull request Nov 14, 2017
* 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 pushed a commit that referenced this pull request Nov 17, 2017
* 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 pushed a commit that referenced this pull request Nov 17, 2017
* 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 mentioned this pull request Nov 21, 2017
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
* 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 pushed a commit that referenced this pull request Nov 28, 2017
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants