Skip to content

test: add coverage for vm: Module and SyntheticModule#31028

Closed
andrewkuzmenko wants to merge 2 commits intonodejs:masterfrom
andrewkuzmenko:vm-module-basic-throws
Closed

test: add coverage for vm: Module and SyntheticModule#31028
andrewkuzmenko wants to merge 2 commits intonodejs:masterfrom
andrewkuzmenko:vm-module-basic-throws

Conversation

@andrewkuzmenko
Copy link

@andrewkuzmenko andrewkuzmenko commented Dec 19, 2019

code and learn task for additional coverage for situation when
creating an abstract instance of the Module and SyntheticModule throws exception if invalid params in constructor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 19, 2019
lpinca
lpinca approved these changes Dec 19, 2019
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 20, 2019

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

Andrew Kuzmenko added 2 commits December 20, 2019 12:28
test: cover vm with negative tests
3735341 
Test the impossibility of creating
an abstract instance of the Module.
Test of SyntheticModule to throw exception
if invalid params in constructor
test: fix eslint and use common module for expectsError
d2ff9fa 
Fix 'max-len','no-restricted-syntax'
(assert.throws must be envoken with at least two args)
and useless blank lines at the end of file
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 20, 2019

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 20, 2019

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

@BridgeAR
Copy link
Member

BridgeAR commented Dec 20, 2019

Still LGTM

@andrewkuzmenko thanks for rebasing so quickly after the conflict! Just as a note: we mostly care about the error code and roughly check that the error message is indeed about the correct property. I personally suggest to use assert.throws() in the future instead of common.expectsError(). It was originally used at times when assert was not as powerful and as callback variation. The only real difference is the magically type property that common.expectsError supports.

@Trott
Copy link
Member

Trott commented Dec 22, 2019

Landed in ee14a64.

Thanks for the contribution! 🎉

@Trott Trott closed this Dec 22, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 22, 2019
@Trott
test: cover vm with negative tests
ee14a64 
Test the impossibility of creating
an abstract instance of the Module.
Test of SyntheticModule to throw exception
if invalid params in constructor

PR-URL: nodejs#31028
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
@BridgeAR
test: cover vm with negative tests
e5980a1 
Test the impossibility of creating
an abstract instance of the Module.
Test of SyntheticModule to throw exception
if invalid params in constructor

PR-URL: #31028
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@targos
test: cover vm with negative tests
b6c815c 
Test the impossibility of creating
an abstract instance of the Module.
Test of SyntheticModule to throw exception
if invalid params in constructor

PR-URL: #31028
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@BethGriggs
test: cover vm with negative tests
07d195a 
Test the impossibility of creating
an abstract instance of the Module.
Test of SyntheticModule to throw exception
if invalid params in constructor

PR-URL: #31028
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@ZYSzys ZYSzys ZYSzys approved these changes

+2 more reviewers

@devnexen devnexen devnexen approved these changes

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@andrewkuzmenko @nodejs-github-bot @BridgeAR @Trott @addaleax @lpinca @devnexen @devsnek @ZYSzys