You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/04/02 08:49:19 UTC
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #303: PROTON-2357: Improve test coverage in url.cpp
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org