test: add cctest for native URL class #12042
Conversation
jasnell
added
dont-land-on-v4.x
whatwg-url
nodejs-github-bot
added
build
mscdex
added
test
test/cctest/test_url.cc
Outdated
|
|
||
| URL simple("https://example.org:81/a/b/c?query#fragment"); | ||
|
|
||
| CHECK_EQ(simple.protocol().compare("https:"), 0); |
There was a problem hiding this comment.
Nit: Why not CHECK_EQ(simple.protocol(), "https:");?
test/cctest/test_url.cc
Outdated
| #include "gtest/gtest.h" | ||
|
|
||
| TEST(URLTest, Simple) { | ||
| using node::url::URL; |
There was a problem hiding this comment.
I’d just put this once at the top level instead of having it in each test, tbh
jasnell
force-pushed
the
native-url-test
branch
2 times, most recently
from
e62c36f
to
6d369a1
Compare
|
Updated! :-) |
test/cctest/test_url.cc
Outdated
| TEST_F(URLTest, Simple) { | ||
| URL simple("https://example.org:81/a/b/c?query#fragment"); | ||
|
|
||
| CHECK_EQ(simple.protocol(), "https:"); |
There was a problem hiding this comment.
Just wondering for future reference if it is preferred to use CHECK_EQ instead of EXPECT_EQ?
The difference seems to be that CHECK_EQ will abort straight away without allowing for the rest of the tests to be run, whereas EXPECT_EQ which is non-fatal and would allow for the rest of the tests to be run and report the errors afterwards.
For example, using CHECK_EQ and forcing a check to fail:
[ RUN ] URLTest.Simple
Assertion failed: ((simple.protocol()) == ("httpsss")), function TestBody, file ../test/cctest/test_url.cc, line 23.
make: *** [cctest] Abort trap: 6
Compared to EXPECT_EQ, and forcing a the expectation to fail:
[ RUN ] URLTest.Simple
../test/cctest/test_url.cc:24: Failure
Value of: "httpss:"
Expected: simple.protocol()
Which is: "https:"
[ FAILED ] URLTest.Simple (13 ms)
...
[ PASSED ] 36 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] URLTest.Simple
There was a problem hiding this comment.
Good point. I'd actually forgotten that EXPECT_EQ existed :-) I'll switch it up to use that instead.
| @@ -492,7 +492,7 @@ enum url_error_cb_args { | |||
| #define XX(name) name, | |||
| ERR_ARGS(XX) | |||
| #undef XX | |||
| } url_error_cb_args; | |||
| }; | |||
There was a problem hiding this comment.
Is there a reason for this change?
There was a problem hiding this comment.
Yes, when node_url.h is included in the cctest/test_url.h, a compile error occurs because the url_error_cb_args is defined multiple times. I had to make similar changes to the other structs defined in this file. It would likely be good to refactor the node_url.h file a bit if we intend for it to be generally includable in more than just node_url.cc.
jasnell
force-pushed
the
native-url-test
branch
from
6d369a1
to
a94ab47
Compare
|
CI failures are unrelated. |
PR-URL: #12042 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
|
Landed in 51b007a |
|
@jasnell Reopened by accident? |
|
Yep lol. |
|
This needs a manual backport to v7.x |
|
@MylesBorins it will cherry-pick cleanly after #12321. |
Adds a cctest for the new native URL class
Depends on: #11801 and #11956
/cc @bmeck @TimothyGu
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
url, test