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: use strictEqual in test fixtures symlinked #10182

Closed
wants to merge 1 commit into from

Conversation

edsadr
Copy link
Member

@edsadr edsadr commented Dec 8, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

use strictEqual assertions in test fixtures for modules symlinked

@nodejs-github-bot nodejs-github-bot added dont-land-on-v7.x test Issues and PRs related to the tests. labels Dec 8, 2016
@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Dec 8, 2016
assert.equal(foo.dep1.bar.version, 'CORRECT_VERSION');
assert.equal(foo.dep2.bar.version, 'CORRECT_VERSION');
assert.equal(__filename, linkScriptTarget);
const foo = require('./foo');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to the top, along with other requires?

@@ -6,8 +6,8 @@ const path = require('path');
const linkScriptTarget = path.join(common.fixturesDir,
'/module-require-symlink/symlinked.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you are changing this file, I would prefer to write this as

path.join(common.fixturesDir, 'module-require-symlink', 'symlinked.js')

Can you please do this as well?

@edsadr
Copy link
Member Author

edsadr commented Dec 9, 2016

@thefourtheye just did those changes you requested

@@ -1,13 +1,13 @@
'use strict';
const assert = require('assert');
const common = require('../../common');
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the first require in the tests.

use strictEqual assertions in test fixtures for modules symlinked
@edsadr
Copy link
Member Author

edsadr commented Dec 12, 2016

@thefourtheye did the last change too

@italoacasas
Copy link
Contributor

@Fishrock123 it's normal that the bot is trying to add the label on every commit ?

@italoacasas
Copy link
Contributor

@italoacasas
Copy link
Contributor

Landed e467d37

Thanks for the contribution

italoacasas pushed a commit that referenced this pull request Dec 13, 2016
- using strictEqual instead equal
- common dependency should be the first one
- using path.join instead relative path

PR-URL: #10182
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
AnnaMag pushed a commit to AnnaMag/node that referenced this pull request Dec 13, 2016
- using strictEqual instead equal
- common dependency should be the first one
- using path.join instead relative path

PR-URL: nodejs#10182
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
evanlucas pushed a commit that referenced this pull request Dec 14, 2016
- using strictEqual instead equal
- common dependency should be the first one
- using path.join instead relative path

PR-URL: #10182
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
MylesBorins pushed a commit that referenced this pull request Jan 22, 2017
- using strictEqual instead equal
- common dependency should be the first one
- using path.join instead relative path

PR-URL: #10182
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
- using strictEqual instead equal
- common dependency should be the first one
- using path.join instead relative path

PR-URL: #10182
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
- using strictEqual instead equal
- common dependency should be the first one
- using path.join instead relative path

PR-URL: #10182
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants