Feat/pm resolve #4
Conversation
| @@ -647,6 +647,19 @@ RAIIIsolate::~RAIIIsolate() { | |||
| isolate_->Dispose(); | |||
| } | |||
|
|
|||
| #ifdef _WIN32 | |||
There was a problem hiding this comment.
You don't need it. We have a node::kPathSeparator so you can just compare if (c == node::kPathSeparator)
There was a problem hiding this comment.
From what I saw in JS implementation for windows we accept both forward and back slash separator. Does node::kPathSeparator accept both or is one of them not needed?
There was a problem hiding this comment.
node::kPathSeparator is chosen based on the platform, so yes.
| @@ -25,6 +25,6 @@ const { spawnSync } = require('child_process'); | |||
| ); | |||
|
|
|||
| const [fsWrite] = stdout.toString().split('\n'); | |||
| assert.strictEqual(fsWrite, 'false'); | |||
| assert.strictEqual(fsWrite, 'true'); | |||
There was a problem hiding this comment.
We should test the absolute file too
test/cctest/test_util.cc
Outdated
| TEST(UtilTest, PathResolve) { | ||
| #ifdef _WIN32 | ||
| // TODO: figure out how to get env | ||
| EXPECT_EQ(PathResolve(nullptr, {"c:/blah\\blah", "d:/games", "c:../a"}), |
There was a problem hiding this comment.
Wouldn't it be the same as POSIX platforms?
v8::HandleScope handle_scope(isolate_);
Argv argv;
Env env{handle_scope, argv};
There was a problem hiding this comment.
Yes, I was waiting to make it work
| @@ -253,7 +258,7 @@ TEST(UtilTest, MaybeStackBuffer) { | |||
| } | |||
| } | |||
|
|
|||
| TEST(UtilTest, SPrintF) { | |||
| TEST_F(UtilTest, SPrintF) { | |||
There was a problem hiding this comment.
Do you need to change the others to TEST_F? Wouldn't it be just adding a new test with TEST_F?
There was a problem hiding this comment.
It doesn't let you mix both in the same file. I could create a new file but that didn't seem coherent
No description provided.