[
https://issues.apache.org/jira/browse/PROTON-2357?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17313712#comment-17313712
]
ASF GitHub Bot commented on PROTON-2357:
----------------------------------------
jiridanek commented on a change in pull request #303:
URL: https://github.com/apache/qpid-proton/pull/303#discussion_r606137671
##########
File path: cpp/src/url_test.cpp
##########
@@ -111,6 +116,54 @@ TEST_CASE("parse URL","[url]") {
"amqp", "user", "pass", "::1", "1234", "path",
"amqp://user:pass@[::1]:1234/path");
}
+ SECTION("port") {
+ CHECK("host:1234" == url("amqp://host:1234/path").host_port());
+ CHECK(5672 == url("amqp://foo/path").port_int());
+ CHECK(5671 == url("amqps://foo/path").port_int());
+ CHECK(1234 == url("amqps://foo:1234/path").port_int());
+
+ url u("amqps://foo:xyz/path");
+ url_error caught_error("");
+ try{
+ u.port_int();
+ }
+ catch(url_error& err){
+ caught_error=err;
+ }
+ CHECK(std::string("invalid port 'xyz'") ==
std::string(caught_error.what()));
+ }
+ SECTION("misc") {
+ {
+ // url copy constructor
+ url u1("amqp://foo:xyz/path");
+ url u2=u1;
+ CHECK(std::string("amqp://foo:xyz/path") == std::string(u2));
+
+ // url assignment operator
+ url u3("amqp://foo:xyz/pathdontcare");
+ u3=u1;
+ CHECK(std::string("amqp://foo:xyz/path") == std::string(u3));
+ }
+ {
+ url u("amqp://foo:1234/path");
+ CHECK(true == u.empty());
Review comment:
```suggestion
CHECK(u.empty());
```
Similarly as with `if`s, you do not have to write `== true` when checking
for truthiness. But this is a matter of style. If you think that writing `==
true` makes the test clearer, keep doing it. I will not complain in the future.
##########
File path: cpp/src/url_test.cpp
##########
@@ -102,6 +103,10 @@ TEST_CASE("parse URL","[url]") {
CHECK_URL(url("amqps://user%2F%3A=:pass%2F%3A=@example.net/some_topic"),
"amqps", "user/:=", "pass/:=", "example.net", "amqps",
"some_topic",
"amqps://user%2F%3A=:pass%2F%3A=@example.net:amqps/some_topic");
+ // Bad Input
Review comment:
Bad how? Say it's unquoted % at the end of a percent encoded string.
##########
File path: cpp/src/url_test.cpp
##########
@@ -111,6 +116,54 @@ TEST_CASE("parse URL","[url]") {
"amqp", "user", "pass", "::1", "1234", "path",
"amqp://user:pass@[::1]:1234/path");
}
+ SECTION("port") {
+ CHECK("host:1234" == url("amqp://host:1234/path").host_port());
+ CHECK(5672 == url("amqp://foo/path").port_int());
+ CHECK(5671 == url("amqps://foo/path").port_int());
+ CHECK(1234 == url("amqps://foo:1234/path").port_int());
+
+ url u("amqps://foo:xyz/path");
+ url_error caught_error("");
+ try{
+ u.port_int();
+ }
+ catch(url_error& err){
+ caught_error=err;
+ }
+ CHECK(std::string("invalid port 'xyz'") ==
std::string(caught_error.what()));
+ }
+ SECTION("misc") {
+ {
+ // url copy constructor
Review comment:
Don't use braces+comment, when you can create a perfectly good SECTION
```suggestion
SECTION("url copy constructor") {
```
and so on. Here's doc on it, in case you haven't seen a Catch 1 test with
nested sections before
https://github.com/catchorg/Catch2/blob/Catch1.x/docs/tutorial.md#test-cases-and-sections
##########
File path: cpp/src/url_test.cpp
##########
@@ -111,6 +116,54 @@ TEST_CASE("parse URL","[url]") {
"amqp", "user", "pass", "::1", "1234", "path",
"amqp://user:pass@[::1]:1234/path");
}
+ SECTION("port") {
+ CHECK("host:1234" == url("amqp://host:1234/path").host_port());
+ CHECK(5672 == url("amqp://foo/path").port_int());
+ CHECK(5671 == url("amqps://foo/path").port_int());
+ CHECK(1234 == url("amqps://foo:1234/path").port_int());
+
+ url u("amqps://foo:xyz/path");
+ url_error caught_error("");
+ try{
+ u.port_int();
+ }
+ catch(url_error& err){
+ caught_error=err;
+ }
+ CHECK(std::string("invalid port 'xyz'") ==
std::string(caught_error.what()));
+ }
+ SECTION("misc") {
+ {
+ // url copy constructor
+ url u1("amqp://foo:xyz/path");
+ url u2=u1;
+ CHECK(std::string("amqp://foo:xyz/path") == std::string(u2));
Review comment:
```suggestion
CHECK(std::string(u2) == "amqp://foo:xyz/path");
```
Writing your conditions as `LITERAL == VARIABLE`, instead of the other way
around, is considered a good C++ practice in some circles.
(https://stackoverflow.com/questions/370366/why-put-the-constant-before-the-variable-in-a-comparison).
I don't much like it, especially not in tests, and Catch 1 docs does not do it
this way either, but I think it's perfectly fine if you do it here. Especially
because the existing CHECK calls in this file already had this structure.
I think you don't have to do `std::string("amqp://foo:xyz/path")`, a C
literal will work just as well, but I am lazy to try it. Anyways, same as
before, I'd be fine if you keep it the way you have it now, it's just a
thought/suggestion.
##########
File path: cpp/src/url_test.cpp
##########
@@ -111,6 +116,54 @@ TEST_CASE("parse URL","[url]") {
"amqp", "user", "pass", "::1", "1234", "path",
"amqp://user:pass@[::1]:1234/path");
}
+ SECTION("port") {
+ CHECK("host:1234" == url("amqp://host:1234/path").host_port());
+ CHECK(5672 == url("amqp://foo/path").port_int());
+ CHECK(5671 == url("amqps://foo/path").port_int());
+ CHECK(1234 == url("amqps://foo:1234/path").port_int());
+
+ url u("amqps://foo:xyz/path");
+ url_error caught_error("");
+ try{
+ u.port_int();
+ }
+ catch(url_error& err){
+ caught_error=err;
Review comment:
It is helpful to have some autoformatter for source code. Either in your
IDE, or some cli tool (like clang-format). If you spend the time to configure
it to match formatting of the existing code in the project, it will save you
time otherwise spent on chasing spaces around equal signs.
##########
File path: cpp/src/url_test.cpp
##########
@@ -111,6 +116,54 @@ TEST_CASE("parse URL","[url]") {
"amqp", "user", "pass", "::1", "1234", "path",
"amqp://user:pass@[::1]:1234/path");
}
+ SECTION("port") {
+ CHECK("host:1234" == url("amqp://host:1234/path").host_port());
+ CHECK(5672 == url("amqp://foo/path").port_int());
+ CHECK(5671 == url("amqps://foo/path").port_int());
+ CHECK(1234 == url("amqps://foo:1234/path").port_int());
+
+ url u("amqps://foo:xyz/path");
+ url_error caught_error("");
+ try{
+ u.port_int();
+ }
+ catch(url_error& err){
+ caught_error=err;
+ }
+ CHECK(std::string("invalid port 'xyz'") ==
std::string(caught_error.what()));
+ }
+ SECTION("misc") {
Review comment:
Catch allows nesting the sections, so every time you feel the need to
create a `{}` block, and especially a `{}` block with a comment, you can use
nested `SECTION`s for each of the individual cases. See my comment below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [cpp] Improve test coverage in url.cpp
> --------------------------------------
>
> Key: PROTON-2357
> URL: https://issues.apache.org/jira/browse/PROTON-2357
> Project: Qpid Proton
> Issue Type: Test
> Components: cpp-binding
> Reporter: Justin Ross
> Assignee: Justin Ross
> Priority: Major
> Labels: starter
>
> *Assignee: Rakhi Kumari*
> Url.cpp currently has 76% line coverage after running the tests. Increase
> the coverage to 100% or as close as is practical. To do this, add or modify
> tests in url_test.cpp.
> To set up coverage builds:
> # Install lcov, the code coverage tool
> # Configure the build for coverage analysis: cmake
> -DCMAKE_BUILD_TYPE=Coverage [...]
> # Build the code: make build
> # Run the tests: make test
> # Generate coverage results: make coverage
> # View the results at <build>/coverage_results
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
