Skip to content

test: increase coverage for ModuleMap#16045

Closed
robtpaton wants to merge 8 commits intonodejs:masterfrom
robtpaton:increase-coverage
Closed

test: increase coverage for ModuleMap#16045
robtpaton wants to merge 8 commits intonodejs:masterfrom
robtpaton:increase-coverage

Conversation

@robtpaton
Copy link
Contributor

@robtpaton robtpaton commented Oct 6, 2017 •
edited
Loading

Checklist
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
@jasnell
Copy link
Member

jasnell commented Oct 9, 2017

Ping @bmeck

@bmeck
Copy link
Member

bmeck commented Oct 9, 2017

This looks to duplicate a bit of #16061 , but includes an actual test for ModuleJob which the other doesn't. We could merge the files I think. @jasnell do you think we should land as separate or merge the files?

@jasnell
Copy link
Member

jasnell commented Oct 9, 2017

Could be a duplicate. merging the two should be fine.

@Trott
Copy link
Member

Trott commented Oct 11, 2017

@bmeck Could always land now as separate files and anyone who feels strongly about it can merge code in a subsequent PR.

FWIW, I don't mind a lot of similar code in tests if it makes each test more comprehensible as a standalone entity. (For anyone generally interested in why "DRY" may not be desirable in tests, see e.g. https://stackoverflow.com/a/11837973/436641)

@bmeck
Copy link
Member

bmeck commented Oct 11, 2017

@Trott I like it.

@Qard
Copy link
Member

Qard commented Oct 11, 2017

Yep, I'd definitely lean toward clarity of tests over conciseness.

jasnell
jasnell requested changes Oct 13, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Very close. One change that I can see.

test/es-module/test-esm-loader-modulemap.js Outdated
Copy link
Member

@jasnell jasnell Oct 13, 2017

Choose a reason for hiding this comment

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

These should use...

common.expectsError(
  () => moduleMap.get(1),
  {
    code: 'ERR_INVALID_ARG_TYPE',
    type: TypeError,
    message: 'The "url" argument must be of type string'
  }
);

Copy link
Contributor Author

@robtpaton robtpaton Oct 13, 2017

Choose a reason for hiding this comment

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

Hi @jasnell I've update the tests to use common.expectsError as suggested

Thanks!

cjihrig
cjihrig approved these changes Oct 15, 2017
View reviewed changes
test/es-module/test-esm-loader-modulemap.js Outdated
Copy link
Contributor

@cjihrig cjihrig Oct 15, 2017

Choose a reason for hiding this comment

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

Please make this the first require() statement (just move it before require('url')).

@BridgeAR
Copy link
Member

BridgeAR commented Oct 18, 2017

@jasnell I think you forgot to update your LG.

@robtpaton it would be nice if you could rebase to remove the merge commit.

@robtpaton
Copy link
Contributor Author

robtpaton commented Oct 19, 2017

@BridgeAR I've rebased to remove the merge commit

Thanks!
Rob

@Trott
Copy link
Member

Trott commented Oct 27, 2017

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

joyeecheung
joyeecheung requested changes Oct 28, 2017
View reviewed changes
test/es-module/test-esm-loader-modulemap.js Outdated
Copy link
Member

@joyeecheung joyeecheung Oct 28, 2017

Choose a reason for hiding this comment

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

CI failed across all platforms:

not ok 2040 es-module/test-esm-loader-modulemap
  ---
  duration_ms: 0.72
  severity: fail
  stack: |-
    internal/loader/ModuleJob.js:18
        this.modulePromise = this.moduleProvider(url);
                                  ^
    
    TypeError: this.moduleProvider is not a function
        at new ModuleJob (internal/loader/ModuleJob.js:18:31)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/es-module/test-esm-loader-modulemap.js:19:19)
        at Module._compile (module.js:616:30)
        at Object.Module._extensions..js (module.js:627:10)
        at Module.load (module.js:535:32)
        at tryModuleLoad (module.js:478:12)
        at Function.Module._load (module.js:470:3)
        at Function.Module.runMain (module.js:657:10)
        at startup (bootstrap_node.js:191:16)
        at bootstrap_node.js:613:3
  ...

new ModuleJob needs a moduleProvider as the third argument.

Copy link
Member

@addaleax addaleax Oct 29, 2017

Choose a reason for hiding this comment

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

I think you can just use a function that returns a never-resolving Promise for this

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Oct 31, 2017

ping @robtpaton - can you please address @joyeecheung / @addaleax 's comments?

@Trott Trott force-pushed the increase-coverage branch from 65a4ac2 to 9888535 Compare November 1, 2017 13:52
@Trott
Copy link
Member

Trott commented Nov 1, 2017

Rebased, fixed the issue, force-pushed. Hope that's OK, @robtpaton!

PTAL, especially @joyeecheung.

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

@Trott
Copy link
Member

Trott commented Nov 1, 2017 •
edited
Loading

Some build-specific (that is: unrelated to this PR) failures on that last CI. Here's another one:

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

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 1, 2017
@robtpaton @Trott
test: increase coverage for ModuleMap
d1a9c02 
Add test for ModuleMap set with ModuleJob but bad url.

PR-URL: nodejs#16045
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 1, 2017

Landed in d1a9c02.

Thanks for the contribution! 🎉

@Trott Trott closed this Nov 1, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@robtpaton @Qard
test: increase coverage for ModuleMap
cabe716 
Add test for ModuleMap set with ModuleJob but bad url.

PR-URL: nodejs/node#16045
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@robtpaton @Qard
test: increase coverage for ModuleMap
2aac399 
Add test for ModuleMap set with ModuleJob but bad url.

PR-URL: nodejs/node#16045
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@robtpaton
Copy link
Contributor Author

robtpaton commented Nov 2, 2017

Thanks for getting this over the line @Trott

cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@robtpaton @cjihrig
test: increase coverage for ModuleMap
ffe4d7b 
Add test for ModuleMap set with ModuleJob but bad url.

PR-URL: nodejs#16045
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
Merged
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@robtpaton @gibfahn
test: increase coverage for ModuleMap
b3032d2 
Add test for ModuleMap set with ModuleJob but bad url.

PR-URL: #16045
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@robtpaton @addaleax
test: increase coverage for ModuleMap
f8f63fa 
Add test for ModuleMap set with ModuleJob but bad url.

PR-URL: nodejs/node#16045
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax left review comments

@Qard Qard Qard approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+1 more reviewer

@bmeck bmeck bmeck approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. esm Issues and PRs related to the ECMAScript Modules implementation. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.