Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Advance DatabaseDownloder unittest #4021

Closed
wants to merge 1 commit into from
Closed

Advance DatabaseDownloder unittest #4021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/ripple/net/HTTPDownloader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ class HTTPDownloader : public std::enable_shared_from_this<HTTPDownloader>

virtual ~HTTPDownloader() = default;

bool
sessionIsActive() const;

bool
isStopping() const;

protected:
// must be accessed through a shared_ptr
// use make_XXX functions to create
Expand Down Expand Up @@ -88,7 +94,7 @@ class HTTPDownloader : public std::enable_shared_from_this<HTTPDownloader>
std::atomic<bool> stop_;

// Used to protect sessionActive_
std::mutex m_;
mutable std::mutex m_;
bool sessionActive_;
std::condition_variable c_;

Expand Down
14 changes: 14 additions & 0 deletions src/ripple/net/impl/HTTPDownloader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,20 @@ HTTPDownloader::stop()
}
}

bool
HTTPDownloader::sessionIsActive() const
{
std::lock_guard lock(m_);
return sessionActive_;
}

bool
HTTPDownloader::isStopping() const
{
std::lock_guard lock(m_);
return stop_;
}

void
HTTPDownloader::fail(
boost::filesystem::path dstPath,
Expand Down
65 changes: 49 additions & 16 deletions src/test/net/DatabaseDownloader_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

#include <ripple/net/DatabaseDownloader.h>
#include <boost/filesystem/operations.hpp>
#include <boost/predef.h>
#include <condition_variable>
#include <mutex>
#include <test/jtx.h>
Expand All @@ -29,6 +28,8 @@
namespace ripple {
namespace test {

#define REPORT_FAILURE(D) reportFailure(D, __FILE__, __LINE__)

class DatabaseDownloader_test : public beast::unit_test::suite
{
std::shared_ptr<TrustedPublisherServer>
Expand Down Expand Up @@ -65,13 +66,10 @@ class DatabaseDownloader_test : public beast::unit_test::suite
waitComplete()
{
std::unique_lock<std::mutex> lk(m);
using namespace std::chrono_literals;
#if BOOST_OS_WINDOWS
auto constexpr timeout = 4s;
#else
auto constexpr timeout = 2s;
#endif
auto stat = cv.wait_for(lk, timeout, [this] { return called; });

auto stat = cv.wait_for(
lk, std::chrono::seconds(10), [this] { return called; });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that there's no point in minimizing the timeout. The condition_variable should pull us out of the wait before this timeout in any non-failing case. In a failing case it's okay to wait a bit longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, my thoughts exactly


called = false;
return stat;
};
Expand Down Expand Up @@ -103,8 +101,29 @@ class DatabaseDownloader_test : public beast::unit_test::suite
{
return ptr_.get();
}

DatabaseDownloader const*
operator->() const
{
return ptr_.get();
}
};

void
reportFailure(Downloader const& dl, char const* file, int line)
{
std::stringstream ss;
ss << "Failed. LOGS:\n"
<< dl.sink_.messages().str()
<< "\nDownloadCompleter failure."
"\nDatabaseDownloader session active? "
<< std::boolalpha << dl->sessionIsActive()
<< "\nDatabaseDownloader is stopping? " << std::boolalpha
<< dl->isStopping();

fail(ss.str(), file, line);
}

void
testDownload(bool verify)
{
Expand All @@ -122,7 +141,7 @@ class DatabaseDownloader_test : public beast::unit_test::suite
return cfg;
})};

Downloader downloader{env};
Downloader dl{env};

// create a TrustedPublisherServer as a simple HTTP
// server to request from. Use the /textfile endpoint
Expand All @@ -133,7 +152,7 @@ class DatabaseDownloader_test : public beast::unit_test::suite
*this, "downloads", "data", "", false, false};
// initiate the download and wait for the callback
// to be invoked
auto stat = downloader->download(
auto stat = dl->download(
server->local_endpoint().address().to_string(),
std::to_string(server->local_endpoint().port()),
"/textfile",
Expand All @@ -142,12 +161,12 @@ class DatabaseDownloader_test : public beast::unit_test::suite
std::function<void(boost::filesystem::path)>{std::ref(cb)});
if (!BEAST_EXPECT(stat))
{
log << "Failed. LOGS:\n" + downloader.sink_.messages().str();
REPORT_FAILURE(dl);
return;
}
if (!BEAST_EXPECT(cb.waitComplete()))
{
log << "Failed. LOGS:\n" + downloader.sink_.messages().str();
REPORT_FAILURE(dl);
return;
}
BEAST_EXPECT(cb.dest == data.file());
Expand Down Expand Up @@ -187,7 +206,10 @@ class DatabaseDownloader_test : public beast::unit_test::suite
datafile.file(),
std::function<void(boost::filesystem::path)>{
std::ref(cb)}));
BEAST_EXPECT(cb.waitComplete());
if (!BEAST_EXPECT(cb.waitComplete()))
{
REPORT_FAILURE(dl);
}
BEAST_EXPECT(!boost::filesystem::exists(datafile.file()));
BEAST_EXPECTS(
dl.sink_.messages().str().find("async_resolve") !=
Expand All @@ -211,7 +233,10 @@ class DatabaseDownloader_test : public beast::unit_test::suite
11,
datafile.file(),
std::function<void(boost::filesystem::path)>{std::ref(cb)}));
BEAST_EXPECT(cb.waitComplete());
if (!BEAST_EXPECT(cb.waitComplete()))
{
REPORT_FAILURE(dl);
}
BEAST_EXPECT(!boost::filesystem::exists(datafile.file()));
BEAST_EXPECTS(
dl.sink_.messages().str().find("async_connect") !=
Expand All @@ -231,7 +256,10 @@ class DatabaseDownloader_test : public beast::unit_test::suite
11,
datafile.file(),
std::function<void(boost::filesystem::path)>{std::ref(cb)}));
BEAST_EXPECT(cb.waitComplete());
if (!BEAST_EXPECT(cb.waitComplete()))
{
REPORT_FAILURE(dl);
}
BEAST_EXPECT(!boost::filesystem::exists(datafile.file()));
BEAST_EXPECTS(
dl.sink_.messages().str().find("async_handshake") !=
Expand All @@ -251,7 +279,10 @@ class DatabaseDownloader_test : public beast::unit_test::suite
11,
datafile.file(),
std::function<void(boost::filesystem::path)>{std::ref(cb)}));
BEAST_EXPECT(cb.waitComplete());
if (!BEAST_EXPECT(cb.waitComplete()))
{
REPORT_FAILURE(dl);
}
BEAST_EXPECT(!boost::filesystem::exists(datafile.file()));
BEAST_EXPECTS(
dl.sink_.messages().str().find("Insufficient disk space") !=
Expand All @@ -270,6 +301,8 @@ class DatabaseDownloader_test : public beast::unit_test::suite
}
};

#undef REPORT_FAILURE

BEAST_DEFINE_TESTSUITE(DatabaseDownloader, net, ripple);
} // namespace test
} // namespace ripple