Skip to content

test: modify common.fixturesDir to fixtures module - #16095

Closed
emyl3 wants to merge 1 commit into
nodejs:masterfrom
emyl3:elisa/code-learn
Closed

test: modify common.fixturesDir to fixtures module#16095
emyl3 wants to merge 1 commit into
nodejs:masterfrom
emyl3:elisa/code-learn

Conversation

@emyl3

@emyl3 emyl3 commented Oct 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor
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 build Issues and PRs related to Node.js builds or CI infrastructure. libuv Issues and PRs related to the libuv dependency or the uv binding. labels Oct 9, 2017
@emyl3

emyl3 commented Oct 9, 2017

Copy link
Copy Markdown
Contributor Author

hello! I'm new to contributing to node. Could I get some guidance on how to make my PR not have commit 75318e4 included in the changes? Thank you so much :)

@danbev

danbev commented Oct 9, 2017

Copy link
Copy Markdown
Contributor

Could I get some guidance on how to make my PR not have commit 75318e4 included in the changes?

Hi, you can do this by rebasing your working branch with upstream/master:

$ git rebase -i upstream/master

Then delete the the commit by removing this line:

pick 75318e46b8 deps: upgrade libuv to 1.15.0

Then you will be left with only your single commit and you can push your changes to your branch (will require you to force push using git push -f origin code-learn).
Would you like to give that a try and then I can start the continuous integration tests for this PR?

Let me know if the above does not make sense and I'll be happy to help out.

@joyeecheung

joyeecheung commented Oct 11, 2017

Copy link
Copy Markdown
Member

@emyl3 Hi, can you rebase this PR by trying the suggestions in #16095 (comment) ?

@emyl3

emyl3 commented Oct 11, 2017

Copy link
Copy Markdown
Contributor Author

Hi! Thank you for your help. I rebased and then successfully removed the line you mentioned from my commit message but when I force pushed from my local elisa/code-learn branch it said everything was up to date. Please let me know what else I should try! Thank you again :)

BridgeAR
BridgeAR reviewed Oct 18, 2017
View reviewed changes
Comment thread test/parallel/test-module-children.js Outdated
const path = require('path');

const dir = path.join(common.fixturesDir, 'GH-7131');
const dir = path.join(fixtures.fixturesDir, 'GH-7131');

@BridgeAR BridgeAR Oct 18, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use fixtures.path instead.

@jasnell jasnell Oct 18, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Specifically, this would be:

const dir = fixtures.path('GH-7131');

@BridgeAR

BridgeAR commented Oct 18, 2017

Copy link
Copy Markdown
Member

@emyl3 how did you rebase? And did you fetch from upstream?

@emyl3

emyl3 commented Oct 18, 2017

Copy link
Copy Markdown
Contributor Author

@BridgeAR I think I fixed it? Please let me know if this works! Thank you for helping me out!

@gireeshpunathil

gireeshpunathil commented Oct 19, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/10841/
and ping @jasnell @BridgeAR @joyeecheung PTAL

joyeecheung
joyeecheung approved these changes Oct 19, 2017
View reviewed changes
Comment thread test/parallel/test-module-children.js
@@ -1,10 +1,12 @@
// Flags: --no-deprecation

@joyeecheung joyeecheung Oct 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: this change seems unnecessary. Can be removed during landing

@tniessen

tniessen commented Oct 19, 2017

Copy link
Copy Markdown
Member

Landed in ca12ae6, thank you for your contribution! 🎉

@tniessen tniessen closed this Oct 19, 2017
tniessen pushed a commit that referenced this pull request Oct 19, 2017
@emyl3 @tniessen
test: replace fixturesDir with fixtures module
ca12ae6 
PR-URL: #16095
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@emyl3 @MylesBorins
test: replace fixturesDir with fixtures module
dae022e 
PR-URL: #16095
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@emyl3 @addaleax
test: replace fixturesDir with fixtures module
9b14c6e 
PR-URL: nodejs/node#16095
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@emyl3 @addaleax
test: replace fixturesDir with fixtures module
1ed0351 
PR-URL: nodejs/node#16095
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MarcelRaschke MarcelRaschke mentioned this pull request Aug 19, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
@cjihrig cjihrig cjihrig approved these changes
@tniessen tniessen tniessen approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes
@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
+1 more reviewer
@apapirovski apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to Node.js builds or CI infrastructure. libuv Issues and PRs related to the libuv dependency or the uv binding.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.