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

test: remove messages for assert #16814

Closed
wants to merge 2 commits into from
Closed

Conversation

ka3e
Copy link
Contributor

@ka3e ka3e commented Nov 6, 2017

Removes message from assert
Add global comment

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

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels 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
@Trott
Copy link
Member

Trott commented Nov 7, 2017

@Trott
Copy link
Member

Trott commented Nov 7, 2017

Looks like CI may have been having issues. Let's try agian.

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

const assert = require('assert');

// Testing api calls for promises
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.

Can you format the comment as what testing guide suggests?

'use strict';

const common = require('../../common');

// This tests the promise-related n-api calls

const assert = require('assert');
const test_promise = require(`./build/${common.buildType}/test_promise`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

common.mustNotCall());
test_promise.concludeCurrentPromise(Promise.resolve('chained answer'), true);

assert.strictEqual(test_promise.isPromise(promise), true);
Copy link
Member

Choose a reason for hiding this comment

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

This one does not seem to belong here, can you move it back?

Copy link
Member

Choose a reason for hiding this comment

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

@joyeecheung I suggested putting it in there because it uses promise because it was declared in that block. But looking more closely, perhaps we should create a separate promise object for this.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yes, actually it should be assert.strictEqual(test_promise.isPromise(test_promise.createPromise()), true); then it can be safely moved down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it out from the block, created new promise as @joyeecheung suggested


assert.strictEqual(test_promise.isPromise(promise), true);
assert.strictEqual(test_promise.isPromise(test_promise.createPromise()), true);
Copy link
Member

@Trott Trott Nov 8, 2017

Choose a reason for hiding this comment

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

@kkwoker @gabrielschulhof Any idea if this changes the test in a significant/important way? Specifically, I'm not sure if promise having a .then() already attached is significant to this assertion or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Trott I don't think it's significant.

@gireeshpunathil
Copy link
Member

ping @ka3e

@Trott
Copy link
Member

Trott commented Nov 13, 2017

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 13, 2017
* remove custom messages for assert that conceal values
* add comment explaining test
* add block scoping

PR-URL: nodejs#16814
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 13, 2017

Landed in 51f92b6.

Thanks for the contribution! 🎉

@Trott Trott closed this Nov 13, 2017
@ka3e ka3e deleted the contribution branch November 13, 2017 23:02
evanlucas pushed a commit that referenced this pull request Nov 14, 2017
* remove custom messages for assert that conceal values
* add comment explaining test
* add block scoping

PR-URL: #16814
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 13, 2017
* remove custom messages for assert that conceal values
* add comment explaining test
* add block scoping

PR-URL: #16814
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Dec 20, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
* remove custom messages for assert that conceal values
* add comment explaining test
* add block scoping

PR-URL: nodejs#16814
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
* remove custom messages for assert that conceal values
* add comment explaining test
* add block scoping

Backport-PR-URL: #19447
PR-URL: #16814
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants