Advance DatabaseDownloder unittest #4021

undertome · 2021-12-08T22:59:05Z

High Level Overview of Change

This pull request aims to fix spurious failures in the DatabaseDownloader unit test by increasing a timeout value that purports to be the source of the false positives.

Context of Change

A timeout value that determines how long the test waits before indicating failure was increased. The error is not reproducible so, lest the issue recur, this PR introduces additional logging information that would hopefully shed some light on the cause.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Future Tasks

Should the failure persist, consult the log output to assist in diagnosing the issue.

scottschurr

This looks pretty good. However I think there are a few things that could be tidied up in this pull request. I left a few comments, which I'm happy to discuss. I also have a commit that you can look at in case you have questions about what I meant with the comments:

https://github.com/scottschurr/rippled/commits/devon-database-downloader-unittest

Feel free to cherry-pick the commit if you wish, but please don't feel obligated. You may have yet better ways to address the comments I left.

scottschurr · 2021-12-11T01:52:51Z

src/test/net/DatabaseDownloader_test.cpp

-            auto stat = cv.wait_for(lk, timeout, [this] { return called; });
+
+            auto stat = cv.wait_for(
+                lk, std::chrono::seconds(10), [this] { return called; });


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.

Yup, my thoughts exactly

scottschurr · 2021-12-11T02:37:25Z

src/test/net/DatabaseDownloader_test.cpp

@@ -143,6 +140,12 @@ class DatabaseDownloader_test : public beast::unit_test::suite
        if (!BEAST_EXPECT(stat))
        {
            log << "Failed. LOGS:\n" + downloader.sink_.messages().str();


I'm gonna be really picky here. Sorry. I find it's useful to name things the same when they perform the same functions. Here I'm noticing that the Downloader is named downloader in testDownload. But it is consistently named dl in testFailures. I'd prefer that they have the same name throughout the file.

Conventions like this help to reduce brain fatigue. Think about how we consistently name all instances of Env env unless there's a really good reason not to.

Of the two names being used I have a slight preference for downloader. However I suspect you prefer dl. Either will be okay. In my opinion being consistent is more important in this case.

I'm gonna insist on it, I just think it's a really good idea.

You remember my predilection for terse variable names.

scottschurr · 2021-12-13T18:22:45Z

src/test/net/DatabaseDownloader_test.cpp

+                log << "DownloadCompleter::waitComplete timed out. "
+                       "DatabaseDownloader session active? "
+                    << std::boolalpha << dl->sessionIsActive()
+                    << " DatabaseDownloader is stopping? " << std::boolalpha
+                    << dl->isStopping() << std::endl;


I'm seeing a lot of repetition in the formatting of the output for these error cases. I think it would be much better if you wrote a function that you could call to format the output.

scottschurr · 2021-12-13T18:23:23Z

src/ripple/net/HTTPDownloader.h

+    bool
+    sessionIsActive();
+
+    bool
+    isStopping();


These are not const correct. They should be bool methodName() const;. In order to do that you'll need to make member variable m_ mutable. That was an oversight in the original design. m_ should have been mutable all along.

I considered making these things const and mutable then decided against it

scottschurr · 2021-12-13T18:24:32Z

src/test/net/DatabaseDownloader_test.cpp

+            log << "DownloadCompleter::waitComplete timed out. "
+                   "DatabaseDownloader session active? "
+                << std::boolalpha << downloader->sessionIsActive()
+                << " DatabaseDownloader is stopping? " << std::boolalpha
+                << downloader->isStopping() << std::endl;


We have a really hard time getting access to unit test output in CI. The only thing I know we consistently can see is fail output. So my inclination is to turn these log writes into fail writes. It would look something like this:

std::stringstream ss; ss << "\nDownloadCompleter::waitComplete timed out." "\nDatabaseDownloader session active? " << std::boolalpha << downloader->sessionIsActive() << "\nDatabaseDownloader is stopping? " << std::boolalpha << downloader->isStopping(); fail(ss.str(), file, line);

I think this change would make it much more likely we would see the output if we get CI failures in the future.

scottschurr · 2021-12-13T23:02:18Z

src/test/net/DatabaseDownloader_test.cpp

-                << " DatabaseDownloader is stopping? " << std::boolalpha
-                << downloader->isStopping() << std::endl;
-
+            log << "Failed. LOGS:\n" + dl.sink_.messages().str();


This is now the only write in this file going to log, the rest are going to fail messages. And it lost much of what was being reported before. Is all of this intentional?

This isn't a Downloader callback failure, so I wasn't compelled to provide the additional log information.

Made the failure reporting function more generic and included it here as well. Done

scottschurr · 2021-12-14T17:20:02Z

src/test/net/DatabaseDownloader_test.cpp

@@ -29,6 +28,8 @@
 namespace ripple {
 namespace test {

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


After a night's sleep, I have a couple of suggestions regarding the REPORT_FAILURE macro.

A) Macros work by text substitution. Since you're including the semicolon in your macro, the places that invoke your macro and include the terminating semicolon actually have two semicolons — the explicit semicolon plus the one from the macro. That's not the end of the world.

However this also allows the person who's maintaining the code to accidentally omit the explicit closing semicolon. That's also not the end of the world, but it looks weird to other people reading the code.

You can avoid all these concerns by removing the closing semicolon from the REPORT_FAILURE macro. Consider it.

B) Macros have no scope. So once they are defined they affect everything that follows in the transaction unit. We usually expect a translation unit to end at the conclusion a *.cpp file. So we don't expect a macro that's defined below the #includes in a *.cpp file to leak into any other files.

However rippled can optionally be built in a unity style. For unity builds multiple *.cpp files are concatenated into a single translation unit. Suddenly the macro you thought was only affecting your own *.cpp file has leaked into other *.cpp files. That usually doesn't create any problems. But when it does those kinds of problems can be really hard to figure out.

The way to avoid complications like this in unity builds is to explicitly "scope" your macro. I suggest you #undef REPORT_FAILURE near the end of the file. This way the macro will not be able to leak into other files.

Suggestions Summary:

Remove the closing semicolon from the REPORT_FAILURE macro.

#undef REPORT_FAILURE toward the bottom of the file.

Sorry I didn't catch these earlier.

scottschurr

👍 LGTM. Thanks for addressing this.

undertome · 2021-12-21T17:56:51Z

Squashed PR commits and rebased onto HEAD of develop

seelabs

👍 LGTM

seelabs

One nit: we should improve the commit message. "Advance DatabaseDownloader unittest" doesn't describe what's in the patch or the motivation for the patch.

undertome · 2022-01-03T18:53:03Z

One nit: we should improve the commit message. "Advance DatabaseDownloader unittest" doesn't describe what's in the patch or the motivation for the patch.

Done

undertome added Bug Tech Debt Non-urgent improvements labels Dec 8, 2021

undertome requested review from seelabs and scottschurr December 8, 2021 22:59

undertome assigned seelabs Dec 8, 2021

scottschurr self-assigned this Dec 13, 2021

scottschurr reviewed Dec 13, 2021

View reviewed changes

scottschurr reviewed Dec 14, 2021

View reviewed changes

scottschurr approved these changes Dec 14, 2021

View reviewed changes

undertome force-pushed the database-downloader-unittest branch from b8e53c0 to 543b77e Compare December 21, 2021 17:55

seelabs approved these changes Jan 3, 2022

View reviewed changes

seelabs reviewed Jan 3, 2022

View reviewed changes

Address failures in DatabaseDownloader unittest

dbaa182

undertome force-pushed the database-downloader-unittest branch from 543b77e to dbaa182 Compare January 3, 2022 18:51

ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Apr 11, 2022

This was referenced May 10, 2022

Propose 1.9.1-b1 #4158

Closed

Proposed 1.9.1-b1 #4161

Merged

manojsdoshi closed this in #4161 May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advance DatabaseDownloder unittest #4021

Advance DatabaseDownloder unittest #4021

undertome commented Dec 8, 2021

scottschurr left a comment

scottschurr Dec 11, 2021

undertome Dec 13, 2021

scottschurr Dec 11, 2021

undertome Dec 13, 2021

undertome Dec 13, 2021

scottschurr Dec 13, 2021

undertome Dec 13, 2021

scottschurr Dec 13, 2021

undertome Dec 13, 2021

undertome Dec 13, 2021

scottschurr Dec 13, 2021

undertome Dec 13, 2021

scottschurr Dec 13, 2021

undertome Dec 14, 2021

undertome Dec 14, 2021

scottschurr Dec 14, 2021

undertome Dec 14, 2021

undertome Dec 14, 2021

scottschurr left a comment

undertome commented Dec 21, 2021

seelabs left a comment

seelabs left a comment

undertome commented Jan 3, 2022

Advance DatabaseDownloder unittest #4021

Advance DatabaseDownloder unittest #4021

Conversation

undertome commented Dec 8, 2021

High Level Overview of Change

Context of Change

Type of Change

Future Tasks

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

undertome commented Dec 21, 2021

seelabs left a comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

undertome commented Jan 3, 2022