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-require-resolve: use case insensitive compare #116

Closed

Conversation

piscisaureus
Copy link
Contributor

This is an alternative fix for the issue that f6e5740 had intended to fix.
It's needed because f6e5740 has been reverted in e24fa83.
See also https://github.com/node-forward/node/issues/20 and #100

@indutny or @bnoordhuis, can I get your review on this?

Because node-forward is closed, here's the text:


The patch nodejs/node-v0.x-archive@a05f973 was landed to make some tests pass, but I can't really reconstruct what the motivation was. What I can tell is that the test failure wasn't really related to path.normalize() and path.resolve() [not] changing the drive letter case.

This patch has caused a lot of issues, including nodejs/node-v0.x-archive#7031 and nodejs/node-v0.x-archive#7806 and lots of bikeshedding about whether uppercase is the right case or lowercase.

I propose reverting the offending patch, and just doing what node does everywhere else (which is: path functions don't touch the case) and take another look at the cause of those test failures.

@piscisaureus piscisaureus added the wip Issues and PRs that are still a work in progress. label Dec 8, 2014
@caineio caineio added need info and removed wip Issues and PRs that are still a work in progress. labels Dec 8, 2014
@piscisaureus
Copy link
Contributor Author

@rvagg

The test fixtures dirrectory is derived from the path to the currently
running script, which is itself specified on the command line. That
means that the case of the fixtures dir may not match what the test
expects (when executed on a case-insensitive filesystem).
@bnoordhuis
Copy link
Member

LGTM

piscisaureus added a commit that referenced this pull request Dec 9, 2014
The test fixtures directory is derived from the path to the currently
running script, which is itself specified on the command line. That
means that the case of the fixtures dir may not match what the test
expects (when executed on a case-insensitive file system).

PR-URL: #116
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cjihrig
Copy link
Contributor

cjihrig commented Dec 9, 2014

Closing. This landed in dd7ce69

@cjihrig cjihrig closed this Dec 9, 2014
piscisaureus added a commit to piscisaureus/node that referenced this pull request Dec 23, 2014
The test fixtures directory is derived from the path to the currently
running script, which is itself specified on the command line. That
means that the case of the fixtures dir may not match what the test
expects (when executed on a case-insensitive file system).

PR-URL: nodejs/node#116
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
piscisaureus added a commit to piscisaureus/node that referenced this pull request Dec 23, 2014
The test fixtures directory is derived from the path to the currently
running script, which is itself specified on the command line. That
means that the case of the fixtures dir may not match what the test
expects (when executed on a case-insensitive file system).

PR-URL: nodejs/node#116
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
dgozman pushed a commit to dgozman/node that referenced this pull request Oct 13, 2020
process: update v8 fast api calls usage

This commit removes the WrapperTraits specialization for FastHrtime
according to recent changes in the V8 API.

Refs: nodejs#33374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants