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: minor fixes to test-module-loading.js #12728

Closed
wants to merge 1 commit into from

Conversation

ywalterh
Copy link
Contributor

Use block scope for local variables only used in a small code block.
Fixed inverse arguments in some usages of Assert.strictEqual.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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 Apr 28, 2017
@Trott
Copy link
Member

Trott commented Apr 28, 2017


assert.ok(a.A instanceof Function);
assert.strictEqual('A', a.A());
assert.strictEqual(false, false, 'testing the test program.');
Copy link
Member

Choose a reason for hiding this comment

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

I see this was like this before, but is this supposed to be a comment? It will never fail as-is.

Copy link
Member

Choose a reason for hiding this comment

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

That should probably be in test-assert.js and inside an assert.doesNotThrow(). Especially since this is a first-time contribution, I'm OK leaving it. Maybe we can open an issue for it and label it good first contribution for someone else to fix. :-D

Copy link
Member

Choose a reason for hiding this comment

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

(Or, alternatively, I'd be OK with the line simply being removed from this test.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late response. I got confused with it a little bit too. I agree with Trott this is not gaining coverage for testing module-loading. Putting it in comment will be an example of how to use strictEqual. I'll remove it then.

Copy link
Contributor Author

@ywalterh ywalterh Apr 29, 2017

Choose a reason for hiding this comment

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

The commit has been updated to remove the unnecessary assertion. I also updated the title of the commit to be in compliance with other PRs.

Use block scope for local variables only used in a small code block.
Fixed inverse arguments in some usages of Assert.strictEqual.
@ywalterh ywalterh changed the title minor fixes to test-module-loading.js test: minor fixes to test-module-loading.js Apr 29, 2017
@ywalterh
Copy link
Contributor Author

ywalterh commented May 2, 2017

@Trott Hi Rich, I've updated the commit associated with the pull request. Is there any steps I missed for this PR to go through? Again thank you for you help!

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.

LGTM if CI is green

@Trott
Copy link
Member

Trott commented May 2, 2017

Trott pushed a commit to Trott/io.js that referenced this pull request May 2, 2017
Use block scope for local variables only used in a small code block.
Fixed inverse arguments in some usages of Assert.strictEqual.

PR-URL: nodejs#12728
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented May 2, 2017

Landed in 133fb0c.
Thanks for the contribution! 🎉

@Trott Trott closed this May 2, 2017
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
Use block scope for local variables only used in a small code block.
Fixed inverse arguments in some usages of Assert.strictEqual.

PR-URL: nodejs#12728
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants