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: add url type check in Module options #18664

Closed
wants to merge 1 commit into from
Closed

test: add url type check in Module options #18664

wants to merge 1 commit into from

Conversation

gideontee
Copy link
Contributor

Hi. this is my first time contributing to an open source project. I'm a newbie here.
From the Node.js Nightly Code Coverage, I noticed that
the code coverage in root/internal/vm/Module.js lacked some test coverage
for the url options parameter. The test just adds a check to ensure an error is thrown.

The change is a relatively small one. I took a look at the other lack of test areas for that
file, but perhaps I'm not familiar with the codebase to know how to test those.

Any help would be appreciated.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • 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 Feb 8, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 9, 2018

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Feb 9, 2018
@gideontee
Copy link
Contributor Author

gideontee commented Feb 9, 2018

Hey.. thanks for the approval.
What are the steps after this? I referred to https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#landing-pull-requests, but I felt it was quite complicated.

Are CI tests triggered automatically after every PR is made?
P.S. I just updated my name in git config, how do I reflect the changes here?

Thanks!

@devsnek
Copy link
Member

devsnek commented Feb 9, 2018

@jiaherrt ci runs are triggered manually by collaborators (in this case @BridgeAR did it). if you want to amend a commit you can use git commit --amend (see more info here)

The code coverage in `root/internal/vm/Module.js` lacked test coverage
for the url options paramter. The test adds a check to ensure error
is thrown.
@BridgeAR
Copy link
Member

BridgeAR commented Feb 9, 2018

@jiaherrt right now you do not have to do anything further. This is going to stay open a bit longer and if no one else disagrees a collaborator is going to land this commit at some point.

@gideontee
Copy link
Contributor Author

Alright. thanks!

@BridgeAR
Copy link
Member

BridgeAR commented Feb 9, 2018

My pleasure.

@gideontee
Copy link
Contributor Author

Is there any recommended direction to get more involved?

@BridgeAR
Copy link
Member

BridgeAR commented Feb 9, 2018

We have a few issues marked as good first contribution. Besides that I recommend to just look around in code that you are interested in. Writing some tests in the beginning and improving the coverage with that is probably a good start to get used to the code base and to get to know more about how everything works together.

@Trott might have some other tasks as well.

Right now I am too tired to be able to think of any other further tips though. @devsnek ?

@devsnek
Copy link
Member

devsnek commented Feb 9, 2018

i'm probably not a good person to ask, my first pr was so controversial it got elevated to a tc39 meeting topic 😆.

i generally just find either

  • existing code that i don't like how it works
  • some thing i want node to have

then i just make as much of it as i can and open a pr, then node collabs will help you get the pr to where it needs to be to be merged.

if you're new to programming with node.js or programming in general sticking to the link that bridgear can be really good for easing you into both the node ecosystem and our codebase before you start making larger changes.

generally if you're new someone in the collab team who is interested in what you're changing will kinda mentor you. even if your pr gets rejected your changes can inspire other prs that might really improve node.

@gideontee
Copy link
Contributor Author

gideontee commented Feb 9, 2018

Alright. thanks again.
I think my git --amend to change my name in the commit caused the failures.

@devsnek
Copy link
Member

devsnek commented Feb 10, 2018

ci lite for the amended commit: https://ci.nodejs.org/job/node-test-pull-request-lite/166/

@devsnek
Copy link
Member

devsnek commented Feb 10, 2018

landed in edffad0

@devsnek devsnek closed this Feb 10, 2018
devsnek pushed a commit that referenced this pull request Feb 10, 2018
The code coverage in `root/internal/vm/Module.js` lacked test coverage
for the url options paramter. The test adds a check to ensure error
is thrown.

PR-URL: #18664
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gideontee gideontee deleted the vm-test-url branch February 10, 2018 03:36
@Trott
Copy link
Member

Trott commented Feb 13, 2018

@Trott might have some other tasks as well.

https://www.nodetodo.org/next-steps/

@MylesBorins
Copy link
Contributor

This will need to be manually backported to v9.x.

addaleax pushed a commit to addaleax/node that referenced this pull request Feb 27, 2018
The code coverage in `root/internal/vm/Module.js` lacked test coverage
for the url options paramter. The test adds a check to ensure error
is thrown.

PR-URL: nodejs#18664
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Feb 27, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
The code coverage in `root/internal/vm/Module.js` lacked test coverage
for the url options paramter. The test adds a check to ensure error
is thrown.

PR-URL: nodejs#18664
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants