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: modify common.fixturesDir to fixtures module #16095

Closed
wants to merge 1 commit into from

Conversation

emyl3
Copy link
Contributor

@emyl3 emyl3 commented Oct 9, 2017

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

emyl3 commented Oct 9, 2017

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
Copy link
Contributor

danbev commented Oct 9, 2017

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
Copy link
Member

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

@emyl3
Copy link
Contributor Author

emyl3 commented Oct 11, 2017

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 :)

const assert = require('assert');
const path = require('path');

const dir = path.join(common.fixturesDir, 'GH-7131');
const dir = path.join(fixtures.fixturesDir, 'GH-7131');
Copy link
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.

Copy link
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
Copy link
Member

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

@emyl3
Copy link
Contributor Author

emyl3 commented Oct 18, 2017

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

@gireeshpunathil
Copy link
Member

@@ -1,10 +1,12 @@
// Flags: --no-deprecation

Copy link
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
Copy link
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
PR-URL: #16095
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
PR-URL: #16095
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
PR-URL: nodejs/node#16095
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#16095
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

Successfully merging this pull request may close these issues.