FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: update test-npm to use absolute paths for tmp/cache/prefix by iarna · Pull Request #3309 · nodejs/node · GitHub

/ node Public

test: update test-npm to use absolute paths for tmp/cache/prefix - #3309

Closed
iarna wants to merge 1 commit into
nodejs:masterfrom
npm:npm-tests-absolute
ConversationCommits (1)ChecksFiles changed
Closed

test: update test-npm to use absolute paths for tmp/cache/prefix#3309
iarna wants to merge 1 commit into
nodejs:masterfrom
npm:npm-tests-absolute

Conversation

iarna commented Oct 10, 2015

Copy link
Copy Markdown
Member

This gives npm absolute paths to use for its cache, prefix and tmp folders. Using relative paths seems to work, but makes me feel veeery edgy, as we do sometimes run subshells with copies of npm and we do sometimes change the working directory in tests. It seems to me that using relative paths here is just asking for really hard to track down trouble.

r: @Fishrock123
r: @chrisdickinson

mscdex added npm Issues and PRs related to the npm client dependency or the npm registry. tools Issues and PRs related to the tools directory. labels Oct 10, 2015
Fishrock123 reviewed Oct 10, 2015
View reviewed changes
Comment thread tools/test-npm.sh

Fishrock123 Oct 10, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you also s/varibles/variables while you're at it? :D

rvagg commented Oct 12, 2015

Copy link
Copy Markdown
Member

lgtm pending @Fishrock123's request

Fishrock123 commented Oct 20, 2015

Copy link
Copy Markdown
Contributor

Ping @iarna

LGTM, but the title exceeds 50 characters. Also the commit prefix should probably be tools:, or tools,test:.

These are minor nits I can fix on landing if need be, though. :)

Fishrock123 commented Oct 22, 2015

Copy link
Copy Markdown
Contributor

Landed in f5445db with the above nits fixed

Fishrock123 pushed a commit that referenced this pull request Oct 22, 2015
tools: use absolute paths in test-npm
f5445db 
Updated test-npm to use absolute paths for tmp/cache/prefix

PR-URL: #3309
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
rvagg pushed a commit that referenced this pull request Oct 22, 2015
tools: use absolute paths in test-npm
b4f4c24 
Updated test-npm to use absolute paths for tmp/cache/prefix

PR-URL: #3309
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins commented Oct 23, 2015

Copy link
Copy Markdown
Contributor

should this be added to LTS?

/cc @jasnell

Fishrock123 commented Oct 23, 2015

Copy link
Copy Markdown
Contributor

It could be, but I'm not worried about this being necessary in npm@2.

MylesBorins commented Oct 23, 2015

Copy link
Copy Markdown
Contributor

fair enough.

I guess we can hold off on this assuming npm3 will not be backported

Fishrock123 commented Oct 23, 2015

Copy link
Copy Markdown
Contributor

@thealphanerd npm @ 3 is a breaking change. :)

jasnell commented Oct 23, 2015

Copy link
Copy Markdown
Member

Don't see a pressing need to get this into lts
On Oct 23, 2015 12:22 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

@thealphanerd https://github.com/TheAlphaNerd npm @ 3 is a breaking
change. :)

—
Reply to this email directly or view it on GitHub
#3309 (comment).

This was referenced Oct 27, 2015
Merged
Closed
rvagg mentioned this pull request Dec 17, 2015
Closed
MylesBorins mentioned this pull request Jan 14, 2016
Closed
This was referenced Feb 20, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Open
snyk-bot mentioned this pull request Sep 25, 2022
Open
wedataintelligence mentioned this pull request Sep 26, 2022
Open
137717unity mentioned this pull request Sep 26, 2022
Open
snyk-bot mentioned this pull request Sep 27, 2022
Open
wedataintelligence mentioned this pull request Sep 29, 2022
Open
This was referenced Sep 29, 2022
Open
Open
This was referenced Oct 5, 2022
Open
Open
snyk-bot mentioned this pull request Oct 6, 2022
Open
137717unity mentioned this pull request Oct 7, 2022
Open
wedataintelligence mentioned this pull request Oct 7, 2022
Open
This was referenced Oct 8, 2022
Open
Open
137717unity mentioned this pull request Oct 9, 2022
Open
snyk-bot mentioned this pull request Oct 9, 2022
Open
kissofire mentioned this pull request Jun 25, 2023
Open
This was referenced May 13, 2024
Open
Open
This was referenced May 14, 2024
Open
Open
Open
leonardoadame mentioned this pull request May 14, 2024
Open
This was referenced May 14, 2024
Open
Open
Open
This was referenced May 22, 2024
Open
Open
This was referenced May 23, 2024
Open
Open
wraithgar deleted the npm-tests-absolute branch June 26, 2025 15:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

npm Issues and PRs related to the npm client dependency or the npm registry. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL