From f15f1965fc506b5fc08ec5b8cc6869ae68cc110e Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Sun, 4 Feb 2018 13:58:17 -0800 Subject: [PATCH 1/6] Add ResourceRetriever::getFilePath() --- dart/common/LocalResourceRetriever.cpp | 27 +++++++++++++------ dart/common/LocalResourceRetriever.hpp | 3 +++ dart/common/ResourceRetriever.cpp | 6 +++++ dart/common/ResourceRetriever.hpp | 19 ++++++++----- dart/utils/CompositeResourceRetriever.cpp | 13 +++++++++ dart/utils/CompositeResourceRetriever.hpp | 3 +++ dart/utils/DartResourceRetriever.cpp | 33 +++++++++++++++++++++++ dart/utils/DartResourceRetriever.hpp | 3 +++ dart/utils/PackageResourceRetriever.cpp | 30 ++++++++++++++------- dart/utils/PackageResourceRetriever.hpp | 3 +++ 10 files changed, 115 insertions(+), 25 deletions(-) diff --git a/dart/common/LocalResourceRetriever.cpp b/dart/common/LocalResourceRetriever.cpp index f589bb35c8689..a9ec3cd7c4dcb 100644 --- a/dart/common/LocalResourceRetriever.cpp +++ b/dart/common/LocalResourceRetriever.cpp @@ -43,14 +43,7 @@ namespace common { //============================================================================== bool LocalResourceRetriever::exists(const Uri& _uri) { - // Open and close the file to check if it exists. It would be more efficient - // to stat() it, but that is not portable. - if(_uri.mScheme.get_value_or("file") != "file") - return false; - else if (!_uri.mPath) - return false; - - return std::ifstream(_uri.getFilesystemPath(), std::ios::binary).good(); + return !getFilePath(_uri).empty(); } //============================================================================== @@ -70,5 +63,23 @@ common::ResourcePtr LocalResourceRetriever::retrieve(const Uri& _uri) return nullptr; } +//============================================================================== +std::string LocalResourceRetriever::getFilePath(const Uri& uri) +{ + // Open and close the file to check if it exists. It would be more efficient + // to stat() it, but that is not portable. + if(uri.mScheme.get_value_or("file") != "file") + return ""; + else if (!uri.mPath) + return ""; + + const auto path = uri.getFilesystemPath(); + + if (std::ifstream(path, std::ios::binary).good()) + return path; + else + return ""; +} + } // namespace common } // namespace dart diff --git a/dart/common/LocalResourceRetriever.hpp b/dart/common/LocalResourceRetriever.hpp index 1a8de1da50480..b6769bb815934 100644 --- a/dart/common/LocalResourceRetriever.hpp +++ b/dart/common/LocalResourceRetriever.hpp @@ -50,6 +50,9 @@ class LocalResourceRetriever : public virtual ResourceRetriever // Documentation inherited. ResourcePtr retrieve(const Uri& _uri) override; + + // Documentation inherited. + std::string getFilePath(const Uri& uri) override; }; using LocalResourceRetrieverPtr = std::shared_ptr; diff --git a/dart/common/ResourceRetriever.cpp b/dart/common/ResourceRetriever.cpp index 406abb632083d..6e3e0e25e9e67 100644 --- a/dart/common/ResourceRetriever.cpp +++ b/dart/common/ResourceRetriever.cpp @@ -53,5 +53,11 @@ std::string ResourceRetriever::readAll(const Uri& uri) return resource->readAll(); } +//============================================================================== +std::string ResourceRetriever::getFilePath(const Uri& /*uri*/) +{ + return ""; +} + } // namespace common } // namespace dart diff --git a/dart/common/ResourceRetriever.hpp b/dart/common/ResourceRetriever.hpp index 28aa31dc0faab..3cb114e46d817 100644 --- a/dart/common/ResourceRetriever.hpp +++ b/dart/common/ResourceRetriever.hpp @@ -48,10 +48,10 @@ class ResourceRetriever public: virtual ~ResourceRetriever() = default; - /// \brief Return whether the resource specified by a URI exists. + /// Returns whether the resource specified by a URI exists. virtual bool exists(const Uri& _uri) = 0; - /// \brief Return the resource specified by a URI or nullptr on failure. + /// Returns the resource specified by a URI or nullptr on failure. virtual ResourcePtr retrieve(const Uri& _uri) = 0; /// Reads all data from the resource of uri, and returns it as a string. @@ -61,11 +61,16 @@ class ResourceRetriever /// \throw std::runtime_error when failed to read sucessfully. virtual std::string readAll(const Uri& uri); - // We don't const-qualify for exists, retrieve, and readAll here. Derived - // classes of ResourceRetriever will be interacting with external resources - // that you don't necessarily have control over so we cannot guarantee that - // you get the same result every time with the same input Uri. Indeed, - // const-qualification for those functions is pointless. + /// Returns absolute file path to \c uri; an empty string if unavailable. + /// + /// This base class returns an empty string by default. + virtual std::string getFilePath(const Uri& uri); + + // We don't const-qualify for exists, retrieve, readAll, and getFilePath here. + // Derived classes of ResourceRetriever will be interacting with external + // resources that you don't necessarily have control over so we cannot + // guarantee that you get the same result every time with the same input Uri. + // Indeed, const-qualification for those functions is pointless. }; using ResourceRetrieverPtr = std::shared_ptr; diff --git a/dart/utils/CompositeResourceRetriever.cpp b/dart/utils/CompositeResourceRetriever.cpp index 9ae5e28cc2f49..d5b34f73968ef 100644 --- a/dart/utils/CompositeResourceRetriever.cpp +++ b/dart/utils/CompositeResourceRetriever.cpp @@ -100,6 +100,19 @@ common::ResourcePtr CompositeResourceRetriever::retrieve( return nullptr; } +//============================================================================== +std::string CompositeResourceRetriever::getFilePath(const common::Uri& uri) +{ + for (const auto& resourceRetriever : getRetrievers(uri)) + { + const auto path = resourceRetriever->getFilePath(uri); + if (!path.empty()) + return path; + } + + return ""; +} + //============================================================================== std::vector CompositeResourceRetriever::getRetrievers(const common::Uri& _uri) const diff --git a/dart/utils/CompositeResourceRetriever.hpp b/dart/utils/CompositeResourceRetriever.hpp index c37edf390dc6d..286a2b04064e3 100644 --- a/dart/utils/CompositeResourceRetriever.hpp +++ b/dart/utils/CompositeResourceRetriever.hpp @@ -72,6 +72,9 @@ class CompositeResourceRetriever : public virtual common::ResourceRetriever // Documentation inherited. common::ResourcePtr retrieve(const common::Uri& _uri) override; + // Documentation inherited. + std::string getFilePath(const common::Uri& uri) override; + private: std::vector getRetrievers( const common::Uri& _uri) const; diff --git a/dart/utils/DartResourceRetriever.cpp b/dart/utils/DartResourceRetriever.cpp index e52e2d0e2aadf..764cde48fb1db 100644 --- a/dart/utils/DartResourceRetriever.cpp +++ b/dart/utils/DartResourceRetriever.cpp @@ -103,6 +103,39 @@ common::ResourcePtr DartResourceRetriever::retrieve(const common::Uri& uri) return nullptr; } +//============================================================================== +std::string DartResourceRetriever::getFilePath(const common::Uri& uri) +{ + std::string relativePath; + if (!resolveDataUri(uri, relativePath)) + return ""; + + if (uri.mAuthority.get() == "sample") + { + for (const auto& dataPath : mDataDirectories) + { + common::Uri fileUri; + fileUri.fromPath(dataPath + relativePath); + + const auto path = mLocalRetriever->getFilePath(fileUri); + + // path is empty if the file specified by fileUri doesn't exist. + if (!path.empty()) + return path; + } + } + else + { + const auto path = mLocalRetriever->getFilePath(uri); + + // path is empty if the file specified by fileUri doesn't exist. + if (!path.empty()) + return path; + } + + return ""; +} + //============================================================================== void DartResourceRetriever::addDataDirectory( const std::string& dataDirectory) diff --git a/dart/utils/DartResourceRetriever.hpp b/dart/utils/DartResourceRetriever.hpp index e525438b8603d..dc11cb19632e3 100644 --- a/dart/utils/DartResourceRetriever.hpp +++ b/dart/utils/DartResourceRetriever.hpp @@ -84,6 +84,9 @@ class DartResourceRetriever : public common::ResourceRetriever // Documentation inherited. common::ResourcePtr retrieve(const common::Uri& uri) override; + // Documentation inherited. + std::string getFilePath(const common::Uri& uri) override; + private: void addDataDirectory(const std::string& packageDirectory); diff --git a/dart/utils/PackageResourceRetriever.cpp b/dart/utils/PackageResourceRetriever.cpp index 3ca88cecec01b..cb4ecc7cf5318 100644 --- a/dart/utils/PackageResourceRetriever.cpp +++ b/dart/utils/PackageResourceRetriever.cpp @@ -68,38 +68,48 @@ void PackageResourceRetriever::addPackageDirectory( //============================================================================== bool PackageResourceRetriever::exists(const common::Uri& _uri) +{ + return !getFilePath(_uri).empty(); +} + +//============================================================================== +common::ResourcePtr PackageResourceRetriever::retrieve(const common::Uri& _uri) { std::string packageName, relativePath; if (!resolvePackageUri(_uri, packageName, relativePath)) - return false; + return nullptr; for(const std::string& packagePath : getPackagePaths(packageName)) { common::Uri fileUri; fileUri.fromPath(packagePath + relativePath); - if (mLocalRetriever->exists(fileUri)) - return true; + if(const auto resource = mLocalRetriever->retrieve(fileUri)) + return resource; } - return false; + return nullptr; } //============================================================================== -common::ResourcePtr PackageResourceRetriever::retrieve(const common::Uri& _uri) +std::string PackageResourceRetriever::getFilePath(const common::Uri& uri) { std::string packageName, relativePath; - if (!resolvePackageUri(_uri, packageName, relativePath)) - return nullptr; + if (!resolvePackageUri(uri, packageName, relativePath)) + return ""; for(const std::string& packagePath : getPackagePaths(packageName)) { common::Uri fileUri; fileUri.fromPath(packagePath + relativePath); - if(const auto resource = mLocalRetriever->retrieve(fileUri)) - return resource; + const auto path = mLocalRetriever->getFilePath(fileUri); + + // path is empty if the file specified by fileUri doesn't exist. + if (!path.empty()) + return path; } - return nullptr; + + return ""; } //============================================================================== diff --git a/dart/utils/PackageResourceRetriever.hpp b/dart/utils/PackageResourceRetriever.hpp index 91e619778c1cb..a88eca52a792d 100644 --- a/dart/utils/PackageResourceRetriever.hpp +++ b/dart/utils/PackageResourceRetriever.hpp @@ -91,6 +91,9 @@ class PackageResourceRetriever : public virtual common::ResourceRetriever // Documentation inherited. common::ResourcePtr retrieve(const common::Uri& _uri) override; + // Documentation inherited. + std::string getFilePath(const common::Uri& uri) override; + private: common::ResourceRetrieverPtr mLocalRetriever; std::unordered_map > mPackageMap; From deaa6f3d5cc7d1f16f0aaecfc9693554fd926f67 Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Sun, 4 Feb 2018 21:47:19 -0800 Subject: [PATCH 2/6] Revert change of PackageResourceRetriever::exists() --- dart/utils/PackageResourceRetriever.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/dart/utils/PackageResourceRetriever.cpp b/dart/utils/PackageResourceRetriever.cpp index cb4ecc7cf5318..a28e84126e3e4 100644 --- a/dart/utils/PackageResourceRetriever.cpp +++ b/dart/utils/PackageResourceRetriever.cpp @@ -69,7 +69,20 @@ void PackageResourceRetriever::addPackageDirectory( //============================================================================== bool PackageResourceRetriever::exists(const common::Uri& _uri) { - return !getFilePath(_uri).empty(); + std::string packageName, relativePath; + if (!resolvePackageUri(_uri, packageName, relativePath)) + return false; + + for (const std::string& packagePath : getPackagePaths(packageName)) + { + common::Uri fileUri; + fileUri.fromPath(packagePath + relativePath); + + if (mLocalRetriever->exists(fileUri)) + return true; + } + + return false; } //============================================================================== From 9ed3b58e4ce043ee90cc381d03e6065f986d9b50 Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Sun, 4 Feb 2018 21:47:47 -0800 Subject: [PATCH 3/6] Add tests for [~]ResourceRetriver::getFilePath() --- unittests/TestHelpers.hpp | 14 +++ .../unit/test_CompositeResourceRetriever.cpp | 82 +++++++++++++ unittests/unit/test_DartResourceRetriever.cpp | 18 +-- .../unit/test_LocalResourceRetriever.cpp | 35 ++++++ .../unit/test_PackageResourceRetriever.cpp | 115 ++++++++++++++++++ 5 files changed, 256 insertions(+), 8 deletions(-) diff --git a/unittests/TestHelpers.hpp b/unittests/TestHelpers.hpp index ba36a8396243e..247bd3c8440e9 100644 --- a/unittests/TestHelpers.hpp +++ b/unittests/TestHelpers.hpp @@ -505,6 +505,12 @@ struct PresentResourceRetriever : public dart::common::ResourceRetriever return true; } + std::string getFilePath(const dart::common::Uri& _uri) override + { + mGetFilePath.push_back(_uri.toString()); + return _uri.toString(); + } + dart::common::ResourcePtr retrieve(const dart::common::Uri& _uri) override { mRetrieve.push_back(_uri.toString()); @@ -512,6 +518,7 @@ struct PresentResourceRetriever : public dart::common::ResourceRetriever } std::vector mExists; + std::vector mGetFilePath; std::vector mRetrieve; }; @@ -524,6 +531,12 @@ struct AbsentResourceRetriever : public dart::common::ResourceRetriever return false; } + std::string getFilePath(const dart::common::Uri& _uri) override + { + mGetFilePath.push_back(_uri.toString()); + return ""; + } + dart::common::ResourcePtr retrieve(const dart::common::Uri& _uri) override { mRetrieve.push_back(_uri.toString()); @@ -531,6 +544,7 @@ struct AbsentResourceRetriever : public dart::common::ResourceRetriever } std::vector mExists; + std::vector mGetFilePath; std::vector mRetrieve; }; diff --git a/unittests/unit/test_CompositeResourceRetriever.cpp b/unittests/unit/test_CompositeResourceRetriever.cpp index cf5aee97c3a5e..bf3b96c9e76a1 100644 --- a/unittests/unit/test_CompositeResourceRetriever.cpp +++ b/unittests/unit/test_CompositeResourceRetriever.cpp @@ -113,6 +113,88 @@ TEST(CompositeResourceRetriever, exists_DefaultResourceRetrieverSucceeds_Returns EXPECT_TRUE(retriever3->mRetrieve.empty()); } +TEST(CompositeResourceRetriever, getFilePath_NothingRegistered_ReturnsEmptyString) +{ + CompositeResourceRetriever retriever; + EXPECT_EQ(retriever.getFilePath(Uri::createFromString("package://test/foo")), ""); +} + +TEST(CompositeResourceRetriever, getFilePath_AllRetrieversFail_ReturnsFalse) +{ + auto retriever1 = std::make_shared(); + auto retriever2 = std::make_shared(); + CompositeResourceRetriever retriever; + + EXPECT_TRUE(retriever.addSchemaRetriever("package", retriever1)); + retriever.addDefaultRetriever(retriever2); + + EXPECT_EQ(retriever.getFilePath(Uri::createFromString("package://test/foo")), ""); + + EXPECT_TRUE(retriever1->mExists.empty()); + ASSERT_EQ(1u, retriever1->mGetFilePath.size()); + EXPECT_EQ("package://test/foo", retriever1->mGetFilePath.front()); + EXPECT_TRUE(retriever1->mRetrieve.empty()); + + EXPECT_TRUE(retriever2->mExists.empty()); + ASSERT_EQ(1u, retriever2->mGetFilePath.size()); + EXPECT_EQ("package://test/foo", retriever2->mGetFilePath.front()); + EXPECT_TRUE(retriever2->mRetrieve.empty()); +} + +TEST(CompositeResourceRetriever, getFilePath_CompositeResourceRetrieverSucceeds_ReturnsFilePath) +{ + auto retriever1 = std::make_shared(); + auto retriever2 = std::make_shared(); + auto retriever3 = std::make_shared(); + CompositeResourceRetriever retriever; + + EXPECT_TRUE(retriever.addSchemaRetriever("package", retriever1)); + EXPECT_TRUE(retriever.addSchemaRetriever("package", retriever2)); + retriever.addDefaultRetriever(retriever3); + + EXPECT_EQ(retriever.getFilePath(Uri::createFromString("package://test/foo")), "package://test/foo"); + + EXPECT_TRUE(retriever2->mExists.empty()); + ASSERT_EQ(1u, retriever1->mGetFilePath.size()); + EXPECT_EQ("package://test/foo", retriever1->mGetFilePath.front()); + EXPECT_TRUE(retriever1->mRetrieve.empty()); + + EXPECT_TRUE(retriever2->mExists.empty()); + EXPECT_TRUE(retriever2->mGetFilePath.empty()); + EXPECT_TRUE(retriever2->mRetrieve.empty()); + EXPECT_TRUE(retriever3->mExists.empty()); + EXPECT_TRUE(retriever3->mGetFilePath.empty()); + EXPECT_TRUE(retriever3->mRetrieve.empty()); +} + +TEST(CompositeResourceRetriever, getFilePath_DefaultResourceRetrieverSucceeds_ReturnsFilePath) +{ + auto retriever1 = std::make_shared(); + auto retriever2 = std::make_shared(); + auto retriever3 = std::make_shared(); + CompositeResourceRetriever retriever; + + EXPECT_TRUE(retriever.addSchemaRetriever("package", retriever1)); + retriever.addDefaultRetriever(retriever2); + retriever.addDefaultRetriever(retriever3); + + EXPECT_EQ(retriever.getFilePath(Uri::createFromString("package://test/foo")), "package://test/foo"); + + EXPECT_TRUE(retriever1->mExists.empty()); + ASSERT_EQ(1u, retriever1->mGetFilePath.size()); + EXPECT_EQ("package://test/foo", retriever1->mGetFilePath.front()); + EXPECT_TRUE(retriever1->mRetrieve.empty()); + + EXPECT_TRUE(retriever2->mExists.empty()); + ASSERT_EQ(1u, retriever2->mGetFilePath.size()); + EXPECT_EQ("package://test/foo", retriever2->mGetFilePath.front()); + EXPECT_TRUE(retriever2->mRetrieve.empty()); + + EXPECT_TRUE(retriever3->mExists.empty()); + EXPECT_TRUE(retriever3->mGetFilePath.empty()); + EXPECT_TRUE(retriever3->mRetrieve.empty()); +} + TEST(CompositeResourceRetriever, retrieve_NothingRegistered_ReturnsNull) { CompositeResourceRetriever retriever; diff --git a/unittests/unit/test_DartResourceRetriever.cpp b/unittests/unit/test_DartResourceRetriever.cpp index a50e287f5a996..00b53567df0b5 100644 --- a/unittests/unit/test_DartResourceRetriever.cpp +++ b/unittests/unit/test_DartResourceRetriever.cpp @@ -32,12 +32,13 @@ #include #include +#include "dart/config.hpp" #include "dart/utils/DartResourceRetriever.hpp" using namespace dart; //============================================================================== -TEST(DartResourceRetriever, ExistsAndRetrieve) +TEST(DartResourceRetriever, ExistsAndGetFilePathAndRetrieve) { auto retriever = utils::DartResourceRetriever::create(); @@ -47,16 +48,17 @@ TEST(DartResourceRetriever, ExistsAndRetrieve) EXPECT_FALSE(retriever->exists("dart://sample/does/not/exist")); EXPECT_TRUE(retriever->exists("dart://sample/skel/shapes.skel")); + EXPECT_EQ(retriever->getFilePath("unknown://test"), ""); + EXPECT_EQ(retriever->getFilePath("unknown://sample/test"), ""); + EXPECT_EQ(retriever->getFilePath("dart://unknown/test"), ""); + EXPECT_EQ(retriever->getFilePath("dart://sample/does/not/exist"), ""); + EXPECT_EQ(retriever->getFilePath( + "dart://sample/skel/shapes.skel"), + DART_DATA_PATH"skel/shapes.skel"); + EXPECT_EQ(nullptr, retriever->retrieve("unknown://test")); EXPECT_EQ(nullptr, retriever->retrieve("unknown://sample/test")); EXPECT_EQ(nullptr, retriever->retrieve("dart://unknown/test")); EXPECT_EQ(nullptr, retriever->retrieve("dart://sample/does/not/exist")); EXPECT_NE(nullptr, retriever->retrieve("dart://sample/skel/shapes.skel")); } - -//============================================================================== -int main(int argc, char* argv[]) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} diff --git a/unittests/unit/test_LocalResourceRetriever.cpp b/unittests/unit/test_LocalResourceRetriever.cpp index 7def96ed88ece..0982402bbae3e 100644 --- a/unittests/unit/test_LocalResourceRetriever.cpp +++ b/unittests/unit/test_LocalResourceRetriever.cpp @@ -74,6 +74,41 @@ TEST(LocalResourceRetriever, exists_PathDoesExists_ReturnsTrue) EXPECT_TRUE(retriever.exists(DART_DATA_PATH "skel/cube.skel")); } +TEST(LocalResourceRetriever, getFilePath_UnsupportedUri_ReturnsEmptyString) +{ + LocalResourceRetriever retriever; + EXPECT_EQ(retriever.getFilePath("unknown://test"), ""); +} + +TEST(LocalResourceRetriever, getFilePath_FileUriDoesNotExist_ReturnsEmptyString) +{ + LocalResourceRetriever retriever; + EXPECT_EQ( + retriever.getFilePath(FILE_SCHEME DART_DATA_PATH "does/not/exist"), ""); +} + +TEST(LocalResourceRetriever, getFilePath_PathDoesNotExist_ReturnsEmptyString) +{ + LocalResourceRetriever retriever; + EXPECT_EQ(retriever.getFilePath(DART_DATA_PATH "does/not/exist"), ""); +} + +TEST(LocalResourceRetriever, getFilePath_FileUriDoesExists_ReturnsPath) +{ + LocalResourceRetriever retriever; + EXPECT_EQ( + retriever.getFilePath(FILE_SCHEME DART_DATA_PATH "skel/cube.skel"), + DART_DATA_PATH"skel/cube.skel"); +} + +TEST(LocalResourceRetriever, getFilePath_PathDoesExists_ReturnsPath) +{ + LocalResourceRetriever retriever; + EXPECT_EQ( + retriever.getFilePath(DART_DATA_PATH "skel/cube.skel"), + DART_DATA_PATH"skel/cube.skel"); +} + TEST(LocalResourceRetriever, retrieve_UnsupportedUri_ReturnsNull) { LocalResourceRetriever retriever; diff --git a/unittests/unit/test_PackageResourceRetriever.cpp b/unittests/unit/test_PackageResourceRetriever.cpp index c7c75dd5b1064..d77639dcbfe29 100644 --- a/unittests/unit/test_PackageResourceRetriever.cpp +++ b/unittests/unit/test_PackageResourceRetriever.cpp @@ -47,6 +47,7 @@ TEST(PackageResourceRetriever, exists_UnableToResolve_ReturnsFalse) EXPECT_FALSE(retriever.exists(Uri::createFromString("package://test/foo"))); EXPECT_TRUE(mockRetriever->mExists.empty()); + EXPECT_TRUE(mockRetriever->mGetFilePath.empty()); EXPECT_TRUE(mockRetriever->mRetrieve.empty()); } @@ -66,6 +67,7 @@ TEST(PackageResourceRetriever, exists_DelegateFails_ReturnsFalse) EXPECT_FALSE(retriever.exists(Uri::createFromString("package://test/foo"))); ASSERT_EQ(1u, mockRetriever->mExists.size()); EXPECT_EQ(expected, mockRetriever->mExists.front()); + EXPECT_TRUE(mockRetriever->mGetFilePath.empty()); EXPECT_TRUE(mockRetriever->mRetrieve.empty()); } @@ -77,6 +79,7 @@ TEST(PackageResourceRetriever, exists_UnsupportedUri_ReturnsFalse) EXPECT_FALSE(retriever.exists(Uri::createFromString("foo://test/foo"))); EXPECT_TRUE(mockRetriever->mExists.empty()); + EXPECT_TRUE(mockRetriever->mGetFilePath.empty()); EXPECT_TRUE(mockRetriever->mRetrieve.empty()); } @@ -95,6 +98,7 @@ TEST(PackageResourceRetriever, exists_StripsTrailingSlash) EXPECT_TRUE(retriever.exists(Uri::createFromString("package://test/foo"))); ASSERT_EQ(1u, mockRetriever->mExists.size()); EXPECT_EQ(expected, mockRetriever->mExists.front()); + EXPECT_TRUE(mockRetriever->mGetFilePath.empty()); EXPECT_TRUE(mockRetriever->mRetrieve.empty()); } @@ -114,6 +118,7 @@ TEST(PackageResourceRetriever, exists_FirstUriSucceeds) EXPECT_TRUE(retriever.exists(Uri::createFromString("package://test/foo"))); ASSERT_EQ(1u, mockRetriever->mExists.size()); EXPECT_EQ(expected, mockRetriever->mExists.front()); + EXPECT_TRUE(mockRetriever->mGetFilePath.empty()); EXPECT_TRUE(mockRetriever->mRetrieve.empty()); } @@ -136,6 +141,116 @@ TEST(PackageResourceRetriever, exists_FallsBackOnSecondUri) ASSERT_EQ(2u, mockRetriever->mExists.size()); EXPECT_EQ(expected1, mockRetriever->mExists[0]); EXPECT_EQ(expected2, mockRetriever->mExists[1]); + EXPECT_TRUE(mockRetriever->mGetFilePath.empty()); + EXPECT_TRUE(mockRetriever->mRetrieve.empty()); +} + +TEST(PackageResourceRetriever, getFilePath_UnableToResolve_ReturnsEmptyString) +{ + auto mockRetriever = std::make_shared(); + PackageResourceRetriever retriever(mockRetriever); + + EXPECT_EQ(retriever.getFilePath(Uri::createFromString("package://test/foo")), ""); + EXPECT_TRUE(mockRetriever->mExists.empty()); + EXPECT_TRUE(mockRetriever->mGetFilePath.empty()); + EXPECT_TRUE(mockRetriever->mRetrieve.empty()); +} + +TEST(PackageResourceRetriever, getFilePath_DelegateFails_ReturnsEmptyString) +{ + // GTest breaks the string concatenation. +#ifdef _WIN32 + const char* expected = "file:///" "dart://sample/test/foo"; +#else + const char* expected = "file://" "dart://sample/test/foo"; +#endif + + auto mockRetriever = std::make_shared(); + PackageResourceRetriever retriever(mockRetriever); + retriever.addPackageDirectory("test", "dart://sample/test"); + + EXPECT_EQ(retriever.getFilePath( + Uri::createFromString("package://test/foo")), ""); + ASSERT_TRUE(mockRetriever->mExists.empty()); + EXPECT_EQ(expected, mockRetriever->mGetFilePath.front()); + ASSERT_EQ(1u, mockRetriever->mGetFilePath.size()); + EXPECT_TRUE(mockRetriever->mRetrieve.empty()); +} + +TEST(PackageResourceRetriever, getFilePath_UnsupportedUri_ReturnsEmptyString) +{ + auto mockRetriever = std::make_shared(); + PackageResourceRetriever retriever(mockRetriever); + retriever.addPackageDirectory("test", "dart://sample/test"); + + EXPECT_EQ(retriever.getFilePath(Uri::createFromString("foo://test/foo")), ""); + EXPECT_TRUE(mockRetriever->mExists.empty()); + EXPECT_TRUE(mockRetriever->mGetFilePath.empty()); + EXPECT_TRUE(mockRetriever->mRetrieve.empty()); +} + +TEST(PackageResourceRetriever, getFilePath_StripsTrailingSlash) +{ +#ifdef _WIN32 + const char* expected = "file:///" "dart://sample/test/foo"; +#else + const char* expected = "file://" "dart://sample/test/foo"; +#endif + + auto mockRetriever = std::make_shared(); + PackageResourceRetriever retriever(mockRetriever); + retriever.addPackageDirectory("test", "dart://sample/test/"); + + EXPECT_EQ(retriever.getFilePath( + Uri::createFromString("package://test/foo")), expected); + ASSERT_TRUE(mockRetriever->mExists.empty()); + EXPECT_EQ(expected, mockRetriever->mGetFilePath.front()); + ASSERT_EQ(1u, mockRetriever->mGetFilePath.size()); + EXPECT_TRUE(mockRetriever->mRetrieve.empty()); +} + +TEST(PackageResourceRetriever, getFilePath_FirstUriSucceeds) +{ +#ifdef _WIN32 + const char* expected = "file:///" "dart://sample/test1/foo"; +#else + const char* expected = "file://" "dart://sample/test1/foo"; +#endif + + auto mockRetriever = std::make_shared(); + PackageResourceRetriever retriever(mockRetriever); + retriever.addPackageDirectory("test", "dart://sample/test1"); + retriever.addPackageDirectory("test", "dart://sample/test2"); + + EXPECT_EQ(retriever.getFilePath( + Uri::createFromString("package://test/foo")), expected); + ASSERT_TRUE(mockRetriever->mExists.empty()); + EXPECT_EQ(expected, mockRetriever->mGetFilePath.front()); + ASSERT_EQ(1u, mockRetriever->mGetFilePath.size()); + EXPECT_TRUE(mockRetriever->mRetrieve.empty()); +} + +TEST(PackageResourceRetriever, getFilePath_FallsBackOnSecondUri) +{ +#ifdef _WIN32 + const char* expected1 = "file:///" "dart://sample/test1/foo"; + const char* expected2 = "file:///" "dart://sample/test2/foo"; +#else + const char* expected1 = "file://" "dart://sample/test1/foo"; + const char* expected2 = "file://" "dart://sample/test2/foo"; +#endif + + auto mockRetriever = std::make_shared(); + PackageResourceRetriever retriever(mockRetriever); + retriever.addPackageDirectory("test", "dart://sample/test1"); + retriever.addPackageDirectory("test", "dart://sample/test2"); + + EXPECT_EQ(retriever.getFilePath( + Uri::createFromString("package://test/foo")), ""); + ASSERT_TRUE(mockRetriever->mExists.empty()); + ASSERT_EQ(2u, mockRetriever->mGetFilePath.size()); + EXPECT_EQ(expected1, mockRetriever->mGetFilePath[0]); + EXPECT_EQ(expected2, mockRetriever->mGetFilePath[1]); EXPECT_TRUE(mockRetriever->mRetrieve.empty()); } From 872ef693e31fa9f411b05e4e301b8b4918e12b88 Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Sun, 4 Feb 2018 21:51:47 -0800 Subject: [PATCH 4/6] Update CHANGELOG.md --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca7ad0278f0a5..9ffb14fd685f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ### DART 6.4.0 (201X-XX-XX) +* Common + + * Added ResourceRetriever::getFilePath(): [#972](https://github.com/dartsim/dart/pull/972) + * Kinematics/Dynamics * Added lazy evaluation for shape's volume and bounding-box computation: [#959](https://github.com/dartsim/dart/pull/959) From 2e0d73d5fa933ef8ea8b5a8c4d0e3282dc2ce735 Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Mon, 5 Feb 2018 00:55:45 -0800 Subject: [PATCH 5/6] Fix tests of invalid URIs --- .../unit/test_PackageResourceRetriever.cpp | 103 +++++++++--------- 1 file changed, 52 insertions(+), 51 deletions(-) diff --git a/unittests/unit/test_PackageResourceRetriever.cpp b/unittests/unit/test_PackageResourceRetriever.cpp index d77639dcbfe29..84f8fe73287d5 100644 --- a/unittests/unit/test_PackageResourceRetriever.cpp +++ b/unittests/unit/test_PackageResourceRetriever.cpp @@ -31,6 +31,7 @@ */ #include +#include "dart/config.hpp" #include "dart/utils/PackageResourceRetriever.hpp" #include "TestHelpers.hpp" @@ -55,14 +56,14 @@ TEST(PackageResourceRetriever, exists_DelegateFails_ReturnsFalse) { // GTest breaks the string concatenation. #ifdef _WIN32 - const char* expected = "file:///" "dart://sample/test/foo"; + const char* expected = "file:///" DART_DATA_LOCAL_PATH"test/foo"; #else - const char* expected = "file://" "dart://sample/test/foo"; + const char* expected = "file://" DART_DATA_LOCAL_PATH"test/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test"); EXPECT_FALSE(retriever.exists(Uri::createFromString("package://test/foo"))); ASSERT_EQ(1u, mockRetriever->mExists.size()); @@ -75,7 +76,7 @@ TEST(PackageResourceRetriever, exists_UnsupportedUri_ReturnsFalse) { auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test"); EXPECT_FALSE(retriever.exists(Uri::createFromString("foo://test/foo"))); EXPECT_TRUE(mockRetriever->mExists.empty()); @@ -86,14 +87,14 @@ TEST(PackageResourceRetriever, exists_UnsupportedUri_ReturnsFalse) TEST(PackageResourceRetriever, exists_StripsTrailingSlash) { #ifdef _WIN32 - const char* expected = "file:///" "dart://sample/test/foo"; + const char* expected = "file:///" DART_DATA_LOCAL_PATH"test/foo"; #else - const char* expected = "file://" "dart://sample/test/foo"; + const char* expected = "file://" DART_DATA_LOCAL_PATH"test/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test/"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test/"); EXPECT_TRUE(retriever.exists(Uri::createFromString("package://test/foo"))); ASSERT_EQ(1u, mockRetriever->mExists.size()); @@ -105,15 +106,15 @@ TEST(PackageResourceRetriever, exists_StripsTrailingSlash) TEST(PackageResourceRetriever, exists_FirstUriSucceeds) { #ifdef _WIN32 - const char* expected = "file:///" "dart://sample/test1/foo"; + const char* expected = "file:///" DART_DATA_LOCAL_PATH"test1/foo"; #else - const char* expected = "file://" "dart://sample/test1/foo"; + const char* expected = "file://" DART_DATA_LOCAL_PATH"test1/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test1"); - retriever.addPackageDirectory("test", "dart://sample/test2"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test1"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test2"); EXPECT_TRUE(retriever.exists(Uri::createFromString("package://test/foo"))); ASSERT_EQ(1u, mockRetriever->mExists.size()); @@ -125,17 +126,17 @@ TEST(PackageResourceRetriever, exists_FirstUriSucceeds) TEST(PackageResourceRetriever, exists_FallsBackOnSecondUri) { #ifdef _WIN32 - const char* expected1 = "file:///" "dart://sample/test1/foo"; - const char* expected2 = "file:///" "dart://sample/test2/foo"; + const char* expected1 = "file:///" DART_DATA_LOCAL_PATH"test1/foo"; + const char* expected2 = "file:///" DART_DATA_LOCAL_PATH"test2/foo"; #else - const char* expected1 = "file://" "dart://sample/test1/foo"; - const char* expected2 = "file://" "dart://sample/test2/foo"; + const char* expected1 = "file://" DART_DATA_LOCAL_PATH"test1/foo"; + const char* expected2 = "file://" DART_DATA_LOCAL_PATH"test2/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test1"); - retriever.addPackageDirectory("test", "dart://sample/test2"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test1"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test2"); EXPECT_FALSE(retriever.exists(Uri::createFromString("package://test/foo"))); ASSERT_EQ(2u, mockRetriever->mExists.size()); @@ -160,14 +161,14 @@ TEST(PackageResourceRetriever, getFilePath_DelegateFails_ReturnsEmptyString) { // GTest breaks the string concatenation. #ifdef _WIN32 - const char* expected = "file:///" "dart://sample/test/foo"; + const char* expected = "file:///" DART_DATA_LOCAL_PATH"test/foo"; #else - const char* expected = "file://" "dart://sample/test/foo"; + const char* expected = "file://" DART_DATA_LOCAL_PATH"test/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test"); EXPECT_EQ(retriever.getFilePath( Uri::createFromString("package://test/foo")), ""); @@ -181,7 +182,7 @@ TEST(PackageResourceRetriever, getFilePath_UnsupportedUri_ReturnsEmptyString) { auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test"); EXPECT_EQ(retriever.getFilePath(Uri::createFromString("foo://test/foo")), ""); EXPECT_TRUE(mockRetriever->mExists.empty()); @@ -192,14 +193,14 @@ TEST(PackageResourceRetriever, getFilePath_UnsupportedUri_ReturnsEmptyString) TEST(PackageResourceRetriever, getFilePath_StripsTrailingSlash) { #ifdef _WIN32 - const char* expected = "file:///" "dart://sample/test/foo"; + const char* expected = "file:///" DART_DATA_LOCAL_PATH"test/foo"; #else - const char* expected = "file://" "dart://sample/test/foo"; + const char* expected = "file://" DART_DATA_LOCAL_PATH"test/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test/"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test/"); EXPECT_EQ(retriever.getFilePath( Uri::createFromString("package://test/foo")), expected); @@ -212,15 +213,15 @@ TEST(PackageResourceRetriever, getFilePath_StripsTrailingSlash) TEST(PackageResourceRetriever, getFilePath_FirstUriSucceeds) { #ifdef _WIN32 - const char* expected = "file:///" "dart://sample/test1/foo"; + const char* expected = "file:///" DART_DATA_LOCAL_PATH"test1/foo"; #else - const char* expected = "file://" "dart://sample/test1/foo"; + const char* expected = "file://" DART_DATA_LOCAL_PATH"test1/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test1"); - retriever.addPackageDirectory("test", "dart://sample/test2"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test1"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test2"); EXPECT_EQ(retriever.getFilePath( Uri::createFromString("package://test/foo")), expected); @@ -233,17 +234,17 @@ TEST(PackageResourceRetriever, getFilePath_FirstUriSucceeds) TEST(PackageResourceRetriever, getFilePath_FallsBackOnSecondUri) { #ifdef _WIN32 - const char* expected1 = "file:///" "dart://sample/test1/foo"; - const char* expected2 = "file:///" "dart://sample/test2/foo"; + const char* expected1 = "file:///" DART_DATA_LOCAL_PATH"test1/foo"; + const char* expected2 = "file:///" DART_DATA_LOCAL_PATH"test2/foo"; #else - const char* expected1 = "file://" "dart://sample/test1/foo"; - const char* expected2 = "file://" "dart://sample/test2/foo"; + const char* expected1 = "file://" DART_DATA_LOCAL_PATH"test1/foo"; + const char* expected2 = "file://" DART_DATA_LOCAL_PATH"test2/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test1"); - retriever.addPackageDirectory("test", "dart://sample/test2"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test1"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test2"); EXPECT_EQ(retriever.getFilePath( Uri::createFromString("package://test/foo")), ""); @@ -268,14 +269,14 @@ TEST(PackageResourceRetriever, retrieve_DelegateFails_ReturnsNull) { // GTest breaks the string concatenation. #ifdef _WIN32 - const char* expected = "file:///" "dart://sample/test/foo"; + const char* expected = "file:///" DART_DATA_LOCAL_PATH"test/foo"; #else - const char* expected = "file://" "dart://sample/test/foo"; + const char* expected = "file://" DART_DATA_LOCAL_PATH"test/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test"); EXPECT_EQ(nullptr, retriever.retrieve(Uri::createFromString("package://test/foo"))); EXPECT_TRUE(mockRetriever->mExists.empty()); @@ -287,7 +288,7 @@ TEST(PackageResourceRetriever, retrieve_UnsupportedUri_ReturnsNull) { auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test"); EXPECT_EQ(nullptr, retriever.retrieve(Uri::createFromString("foo://test/foo"))); EXPECT_TRUE(mockRetriever->mExists.empty()); @@ -297,14 +298,14 @@ TEST(PackageResourceRetriever, retrieve_UnsupportedUri_ReturnsNull) TEST(PackageResourceRetriever, retrieve_StripsTrailingSlash) { #ifdef _WIN32 - const char* expected = "file:///" "dart://sample/test/foo"; + const char* expected = "file:///" DART_DATA_LOCAL_PATH"test/foo"; #else - const char* expected = "file://" "dart://sample/test/foo"; + const char* expected = "file://" DART_DATA_LOCAL_PATH"test/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test/"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test/"); EXPECT_TRUE(retriever.retrieve(Uri::createFromString("package://test/foo")) != nullptr); EXPECT_TRUE(mockRetriever->mExists.empty()); @@ -315,15 +316,15 @@ TEST(PackageResourceRetriever, retrieve_StripsTrailingSlash) TEST(PackageResourceRetriever, retrieve_FirstUriSucceeds) { #ifdef _WIN32 - const char* expected = "file:///" "dart://sample/test1/foo"; + const char* expected = "file:///" DART_DATA_LOCAL_PATH"test1/foo"; #else - const char* expected = "file://" "dart://sample/test1/foo"; + const char* expected = "file://" DART_DATA_LOCAL_PATH"test1/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test1"); - retriever.addPackageDirectory("test", "dart://sample/test2"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test1"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test2"); EXPECT_TRUE(retriever.retrieve(Uri::createFromString("package://test/foo")) != nullptr); EXPECT_TRUE(mockRetriever->mExists.empty()); @@ -334,17 +335,17 @@ TEST(PackageResourceRetriever, retrieve_FirstUriSucceeds) TEST(PackageResourceRetriever, retrieve_FallsBackOnSecondUri) { #ifdef _WIN32 - const char* expected1 = "file:///" "dart://sample/test1/foo"; - const char* expected2 = "file:///" "dart://sample/test2/foo"; + const char* expected1 = "file:///" DART_DATA_LOCAL_PATH"test1/foo"; + const char* expected2 = "file:///" DART_DATA_LOCAL_PATH"test2/foo"; #else - const char* expected1 = "file://" "dart://sample/test1/foo"; - const char* expected2 = "file://" "dart://sample/test2/foo"; + const char* expected1 = "file://" DART_DATA_LOCAL_PATH"test1/foo"; + const char* expected2 = "file://" DART_DATA_LOCAL_PATH"test2/foo"; #endif auto mockRetriever = std::make_shared(); PackageResourceRetriever retriever(mockRetriever); - retriever.addPackageDirectory("test", "dart://sample/test1"); - retriever.addPackageDirectory("test", "dart://sample/test2"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test1"); + retriever.addPackageDirectory("test", DART_DATA_LOCAL_PATH"test2"); EXPECT_EQ(nullptr, retriever.retrieve(Uri::createFromString("package://test/foo"))); EXPECT_TRUE(mockRetriever->mExists.empty()); From c79de2dd272ffe16db4acc7caa9ab53b4af7318e Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Mon, 5 Feb 2018 01:00:34 -0800 Subject: [PATCH 6/6] Add comment on why an additional slash is required for Windows --- unittests/unit/test_PackageResourceRetriever.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/unittests/unit/test_PackageResourceRetriever.cpp b/unittests/unit/test_PackageResourceRetriever.cpp index 84f8fe73287d5..167542bcab49a 100644 --- a/unittests/unit/test_PackageResourceRetriever.cpp +++ b/unittests/unit/test_PackageResourceRetriever.cpp @@ -54,7 +54,9 @@ TEST(PackageResourceRetriever, exists_UnableToResolve_ReturnsFalse) TEST(PackageResourceRetriever, exists_DelegateFails_ReturnsFalse) { - // GTest breaks the string concatenation. + // Additional slash is required for Windows because Windows file system + // doesn't have a leading slash for an absolute path. + // Reference: https://en.wikipedia.org/wiki/File_URI_scheme#Windows #ifdef _WIN32 const char* expected = "file:///" DART_DATA_LOCAL_PATH"test/foo"; #else