Bump in edifice: ign-common4

[chapulina]

Addresses gazebo-tooling/release-tools#361

[chapulina]

There are lots of failing tests because of the changes in the URI class on ign-common4. Some of them quite confusing. See the changes I needed to make for this test to pass:

diff --git a/src/ClientConfig_TEST.cc b/src/ClientConfig_TEST.cc
index 74c5d24..7b8b001 100644
--- a/src/ClientConfig_TEST.cc
+++ b/src/ClientConfig_TEST.cc
@@ -386,15 +386,21 @@ TEST(ServerConfig, Url)
     ServerConfig srv;
     srv.SetUrl(common::URI("http://banana:8080"));
     EXPECT_EQ("http://banana:8080", srv.Url().Str());
+    EXPECT_EQ("", srv.Url().Path().Str());
     EXPECT_EQ("http", srv.Url().Scheme());
-    EXPECT_EQ("banana:8080", srv.Url().Path().Str());
+    EXPECT_EQ("banana", srv.Url().Authority().Host());
+    EXPECT_EQ(8080, srv.Url().Authority().Port());
   }
 
   // Trailing /
   {
     ServerConfig srv;
     srv.SetUrl(common::URI("http://banana:8080/"));
-    EXPECT_EQ("http://banana:8080", srv.Url().Str());
+    EXPECT_EQ("http://banana:8080/", srv.Url().Str());
+    EXPECT_EQ("/", srv.Url().Path().Str());
+    EXPECT_EQ("http", srv.Url().Scheme());
+    EXPECT_EQ("banana", srv.Url().Authority().Host());
+    EXPECT_EQ(8080, srv.Url().Authority().Port());
   }
 
   // Set from URI
@@ -405,9 +411,12 @@ TEST(ServerConfig, Url)
 
     ServerConfig srv;
     srv.SetUrl(url);
-    EXPECT_EQ("http://banana:8080", srv.Url().Str());
-    EXPECT_EQ("http", srv.Url().Scheme());
+    EXPECT_EQ("http:banana:8080", srv.Url().Str());
     EXPECT_EQ("banana:8080", srv.Url().Path().Str());
+    EXPECT_EQ("http", srv.Url().Scheme());
+    EXPECT_EQ("", srv.Url().Authority().Host());
+    EXPECT_FALSE(srv.Url().Authority().Port());
+    EXPECT_EQ("http:banana:8080", srv.Url().Str());
   }
 }

Among other things, note how the Url().Str() and Url().Path().Str() are different for all 3 test cases, when it looks like they should be the same.

I think the new common::URI API leaves a lot of room for errors. Maybe the Path should be deprecated to prevent confusions like this, I'm not sure. I suggest we revisit that before committing to using ign-common4 on Edifice. CC @mjcarroll @nkoenig

[nkoenig]

There are lots of failing tests because of the changes in the URI class on ign-common4. Some of them quite confusing. See the changes I needed to make for this test to pass:

diff --git a/src/ClientConfig_TEST.cc b/src/ClientConfig_TEST.cc
index 74c5d24..7b8b001 100644
--- a/src/ClientConfig_TEST.cc
+++ b/src/ClientConfig_TEST.cc
@@ -386,15 +386,21 @@ TEST(ServerConfig, Url)
     ServerConfig srv;
     srv.SetUrl(common::URI("http://banana:8080"));
     EXPECT_EQ("http://banana:8080", srv.Url().Str());
+    EXPECT_EQ("", srv.Url().Path().Str());
     EXPECT_EQ("http", srv.Url().Scheme());
-    EXPECT_EQ("banana:8080", srv.Url().Path().Str());
+    EXPECT_EQ("banana", srv.Url().Authority().Host());
+    EXPECT_EQ(8080, srv.Url().Authority().Port());
   }
 
   // Trailing /
   {
     ServerConfig srv;
     srv.SetUrl(common::URI("http://banana:8080/"));
-    EXPECT_EQ("http://banana:8080", srv.Url().Str());
+    EXPECT_EQ("http://banana:8080/", srv.Url().Str());
+    EXPECT_EQ("/", srv.Url().Path().Str());
+    EXPECT_EQ("http", srv.Url().Scheme());
+    EXPECT_EQ("banana", srv.Url().Authority().Host());
+    EXPECT_EQ(8080, srv.Url().Authority().Port());
   }
 
   // Set from URI
@@ -405,9 +411,12 @@ TEST(ServerConfig, Url)
 
     ServerConfig srv;
     srv.SetUrl(url);
-    EXPECT_EQ("http://banana:8080", srv.Url().Str());
-    EXPECT_EQ("http", srv.Url().Scheme());
+    EXPECT_EQ("http:banana:8080", srv.Url().Str());
     EXPECT_EQ("banana:8080", srv.Url().Path().Str());
+    EXPECT_EQ("http", srv.Url().Scheme());
+    EXPECT_EQ("", srv.Url().Authority().Host());
+    EXPECT_FALSE(srv.Url().Authority().Port());
+    EXPECT_EQ("http:banana:8080", srv.Url().Str());
   }
 }

Among other things, note how the Url().Str() and Url().Path().Str() are different for all 3 test cases, when it looks like they should be the same.

I think the new common::URI API leaves a lot of room for errors. Maybe the Path should be deprecated to prevent confusions like this, I'm not sure. I suggest we revisit that before committing to using ign-common4 on Edifice. CC @mjcarroll @nkoenig

The changes all look correct based on the tests.

The Path should not contain the authority and a double forward slash after the scheme is only present if an authority is present. I think the most confusing part of this test is the following:

  // Set from URI
  {
    auto url = common::URI();
    url.SetScheme("http");
    url.Path() = common::URIPath("banana:8080");

Here a Path is set to banana:8080. This is valid, but most people would assume the string banana:8080 is an authority.

[chapulina]

Thank you for the clarification, @nkoenig. The test results make more sense to me now.

Because there was no concept of "authority" up to ign-common3, it's safe to assume that users are currently embedding the authority into the path. This makes me concerned about the migration to ign-common4, because this problem may not always be caught through automated tests, and the migration guide doesn't give a clear suggestion for how to update the code.

One suggestion is to:

• deprecate the URI::Path() functions to hint the user that something has changed
• add alternatives that work just like URI::Path, but have a different name (PathSegments?)
• explain on the migration guide that Authority + PathSegments must be used instead of Path

[codecov]

Codecov Report

Merging #163 (35555b2) into main (0f0b1ee) will increase coverage by 0.07%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
+ Coverage   77.85%   77.93%   +0.07%     
==========================================
  Files          19       20       +1     
  Lines        2606     2619      +13     
==========================================
+ Hits         2029     2041      +12     
- Misses        577      578       +1     

Impacted Files	Coverage Δ
src/FuelClient.cc	70.03% <ø> (-0.10%)	⬇️
src/Helpers.cc	93.33% <93.33%> (ø)
src/LocalCache.cc	80.00% <100.00%> (ø)
src/WorldIdentifier.cc	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f0b1ee...35555b2. Read the comment docs.