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 missing cctest/test_path.cc #52148

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Mar 19, 2024

We weren't running and testing test_path.cc. This change ensures that make cctest runs the test file.

cc @RafaelGSS @nodejs/path

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@anonrig anonrig added fast-track PRs that do not need to wait for 48 hours to land. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 19, 2024
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Mar 19, 2024
Copy link
Contributor

Fast-track has been requested by @anonrig. Please 👍 to approve.

@anonrig anonrig requested a review from RafaelGSS March 19, 2024 00:55
@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 19, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 19, 2024
@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member Author

anonrig commented Mar 19, 2024

It seems the existing tests are failing. cc @RafaelGSS @nodejs/platform-windows

@aduh95
Copy link
Contributor

aduh95 commented Mar 19, 2024

It looks like cctest/test_path.cc doesn't pass on Windows

C:\workspace\node-compile-windows\node\test\cctest\test_path.cc:21
Expected equality of these values:
  PathResolve(*env, {"c:/ignore", "d:\\a/b\\c/d", "\\e.exe"})
    Which is: "\\e.exe"
  "d:\\e.exe"

@StefanStojanovic
Copy link
Contributor

The reason behind this is the PathResolve implementation on Windows. All of the failing cases have one thing in common - the last path in the list starts with a path separator (either \\ or /) thus it is evaluated as an absolute path, which it is not.

I see that those test cases are taken from test-path-resolve.js and that the implementation is taken from path.js. However, I found this difference when comparing the two path resolve implementations: JS code, C++ code. Changing this solved the test failures for me locally.

diff --git a/src/path.cc b/src/path.cc
index 6a56b750d6e..78dd5804fc3 100644
--- a/src/path.cc
+++ b/src/path.cc
@@ -101,7 +101,7 @@ std::string PathResolve(Environment* env,
   const size_t numArgs = paths.size();
   auto cwd = env->GetCwd(env->exec_path());
 
-  for (int i = numArgs - 1; i >= -1 && !resolvedAbsolute; i--) {
+  for (int i = numArgs - 1; i >= -1; i--) {
     std::string path;
     if (i >= 0) {
       path = std::string(paths[I]);

@anonrig anonrig force-pushed the add-missing-cctest branch from 06e0270 to d7dd131 Compare March 19, 2024 13:54
@anonrig
Copy link
Member Author

anonrig commented Mar 19, 2024

Thank you @StefanStojanovic. Updated the pull-request.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 19, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 19, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 21, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 21, 2024
@nodejs-github-bot nodejs-github-bot merged commit 0b67673 into nodejs:main Mar 21, 2024
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 0b67673

@anonrig anonrig deleted the add-missing-cctest branch March 21, 2024 14:37
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52148
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52148
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52148
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
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. build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants