test: add regression test for #567 #569

vasild commented at 5:34 PM on March 24, 2022: contributor

Add a test that would fail, should #567 resurface.

Also, add a comment and dedup a long expression.

vasild requested review from ryanofsky on Mar 24, 2022

vasild requested review from jonatack on Mar 24, 2022

vasild commented at 5:42 PM on March 24, 2022: contributor

How to test this:

Find your own way. Don't let me misguide you.

<details> <summary>don't click here</summary>

run src/qt/test/test_bitcoin-qt and observe the newly added test passing:

PASS   : OptionTests::parametersInteraction()

revert the fix from bitcoin-core/gui#568:

--- i/src/qt/optionsmodel.cpp
+++ w/src/qt/optionsmodel.cpp
@@ -168,13 +168,13 @@ void OptionsModel::Init(bool resetSettings)
         //
         // OptionsModel::Init()
         //     this method, can flip -listen from 1 to 0 if fListen=false
         //
         // AppInitParameterInteraction()
         //     error if -listen=0 and -listenonion=1
-        gArgs.SoftSetBoolArg("-listenonion", false);
+        //gArgs.SoftSetBoolArg("-listenonion", false);
     }

     if (!settings.contains("server")) {
         settings.setValue("server", false);
     }
     if (!gArgs.SoftSetBoolArg("-server", settings.value("server").toBool())) {

and observe the test failing:

FAIL!  : OptionTests::parametersInteraction() 'gArgs.IsArgSet("-listenonion")' returned FALSE. ()
   Loc: [qt/test/optiontests.cpp(53)]

in addition to 2., further comment the first check from the test:

--- i/src/qt/test/optiontests.cpp
+++ w/src/qt/test/optiontests.cpp
@@ -47,14 +47,14 @@ void OptionTests::parametersInteraction()

     const bool expected{false};

     QVERIFY(gArgs.IsArgSet("-listen"));
     QCOMPARE(gArgs.GetBoolArg("-listen", !expected), expected);

-    QVERIFY(gArgs.IsArgSet("-listenonion"));
-    QCOMPARE(gArgs.GetBoolArg("-listenonion", !expected), expected);
+    //QVERIFY(gArgs.IsArgSet("-listenonion"));
+    //QCOMPARE(gArgs.GetBoolArg("-listenonion", !expected), expected);

     QVERIFY(AppInitParameterInteraction(gArgs));

     // cleanup
     settings.remove("fListen");
     QVERIFY(!settings.contains("fListen"));

and observe the second check failing:

Error: Cannot set -listen=0 together with -listenonion=1
FAIL!  : OptionTests::parametersInteraction() 'AppInitParameterInteraction(gArgs)' returned FALSE. ()
   Loc: [qt/test/optiontests.cpp(56)]

</details>

jonatack commented at 7:59 PM on March 24, 2022: member

Concept ACK

vasild commented at 9:55 AM on March 25, 2022: contributor

I am puzzled why on macOS 12 native [gui, system sqlite only] [no depends] the newly added test fails with:

Error: Specified blocks directory "" does not exist.
FAIL!  : OptionTests::parametersInteraction() 'AppInitParameterInteraction(gArgs)' returned FALSE. ()
   Loc: [qt/test/optiontests.cpp(56)]

While on my computer and on 32-bit + dash [gui] [CentOS 8] (at least) the test passes.

:confused:

ryanofsky commented at 5:28 PM on March 30, 2022: contributor

Thank you for following up with this! :partying_face:

I am puzzled why on macOS 12 native [gui, system sqlite only] [no depends] the newly added test fails

This is mysterious, but actually less mysterious than it appears at first, because of a terrible error message on the following line that checks if one directory exists (GetBlocksDirPath), but then prints the error message about a different directory ("-blocksdir"):

https://github.com/bitcoin-core/gui/blob/f4e5d704f24ad9beba4f9a4838be3351551964df/src/init.cpp#L828-L829

I think this error just happens if tests run in a different order, and I would try adding a gArgs.ClearPathCache() call and forced_settings.erase("data"); to the top of the failing test(s).

in src/qt/optionsmodel.cpp:173 in cb018e7586 outdated

 170 | +        //
 171 | +        // OptionsModel::Init()
 172 | +        //     this method, can flip -listen from 1 to 0 if fListen=false
 173 | +        //
 174 | +        // AppInitParameterInteraction()
 175 | +        //     error if -listen=0 and -listenonion=1

jonatack commented at 8:27 PM on March 30, 2022:

nit suggestion

         // OptionsModel::Init()
-        //     this method, can flip -listen from 1 to 0 if fListen=false
+        //     this method can flip -listen from 1 to 0 if fListen=false
         //
         // AppInitParameterInteraction()
-        //     error if -listen=0 and -listenonion=1
+        //     raises an error if -listen=0 and -listenonion=1

vasild commented at 1:41 PM on March 31, 2022:

The comma was intentional. Replaced it by brackets. Took the other suggestion.

in src/qt/test/optiontests.cpp:45 in cb018e7586 outdated

  34 | +void OptionTests::parametersInteraction()
  35 | +{
  36 | +    gArgs.LockSettings([&](util::Settings& s) {
  37 | +        s.forced_settings.erase("listen");
  38 | +        s.forced_settings.erase("listenonion");
  39 | +    });

jonatack commented at 8:34 PM on March 30, 2022:

Added asserts here as a sanity check, maybe good to add in case the defaults in test_main.cpp change.

 void OptionTests::parametersInteraction()
 {
+    QVERIFY(gArgs.IsArgSet("-listen"));
+    QVERIFY(gArgs.IsArgSet("-listenonion"));
     gArgs.LockSettings([&](util::Settings& s) {
         s.forced_settings.erase("listen");
         s.forced_settings.erase("listenonion");
     }

vasild commented at 1:43 PM on March 31, 2022:

The test unsets those options, so it does not care or rely that they are set when it starts. Thus I think those checks are not necessary. I.e. the test will still work even if those options are not set when it starts.

jonatack commented at 8:39 PM on March 30, 2022: member

ACK cb018e75864c973d4d3e9591d7d2347a60aee122 modulo the macOS CI

Note that reverting #568 and running ./src/qt/test/test_bitcoin-qt also prints a couple of Error: Cannot set -listen=0 together with -listenonion=1 messages in the RPCNestedTests and WalletTests output, though the tests don't fail outright.

I could be missing a command line argument to do this, but added the following to make it easier to see if any test failures happened:

--- a/src/qt/test/test_main.cpp
+++ b/src/qt/test/test_main.cpp
@@ -112,6 +112,10 @@ int main(int argc, char* argv[])
         fInvalid = true;
     }
 #endif

+    if (fInvalid) {
+        qWarning("\nThere were errors in some of the tests above.\n");
+    } else {
+        qDebug("\nAll tests executed successfully.\n");
+    }

options: add a comment for -listenonion and dedup a long expression

A followup to https://github.com/bitcoin-core/gui/pull/568

Co-authored-by: Jon Atack <jon@atack.com>

3b82608dd1

vasild force-pushed on Mar 31, 2022

vasild commented at 1:40 PM on March 31, 2022: contributor

cb018e7586...cae12fc803: attempt to fix the CI failure and address suggestions

Invalidates ACK from @jonatack

Thanks for the suggestion, @ryanofsky! I added just gArgs.ClearPathCache() as I think that alone should resolve it. @jonatack I look up the exit status of the test executable (echo $?) and if 1 then search the output for FAIL!, yeah, not very convenient. I too noticed the printout Error: Cannot set -listen=0 together with -listenonion=1 from other tests if the bug resurfaces. I think it is ok.

jonatack commented at 2:42 PM on March 31, 2022: member

ACK cae12fc803bc9595dbb4466b4dbbfa29490ce995 provided the CI is happy

in src/qt/test/optiontests.cpp:34 in cae12fc803 outdated

  29 | @@ -29,3 +30,35 @@ void OptionTests::optionTests()
  30 |      });
  31 |      gArgs.WriteSettingsFile();
  32 |  }
  33 | +
  34 | +void OptionTests::parametersInteraction()

jarolrod commented at 1:27 AM on April 1, 2022:

This name is fine with me, although it implied general parameter interaction testing.

Absolutely not critical, but while here, as this test is regression specific; maybe we could add a comment similar to the one in the test above?

vasild commented at 8:33 AM on April 1, 2022:

Added a comment.

jarolrod commented at 1:28 AM on April 1, 2022: member

ACK cae12fc803bc9595dbb4466b4dbbfa29490ce995

This effectively tests the regression described in #567, and fixed in #568.

test: add regression test for bitcoin-core/gui/issues/567 4d4dca43fc

vasild force-pushed on Apr 1, 2022

vasild commented at 8:33 AM on April 1, 2022: contributor

cae12fc803...4d4dca43fc: add a comment in the test

Invalidates ACKs from @jonatack and @jarolrod

jarolrod commented at 2:59 PM on April 1, 2022: member

reACK 4d4dca43fc

only change since my last review is the addition of a comment for the added test

jarolrod added the label Tests on Apr 1, 2022

jonatack commented at 10:45 PM on April 2, 2022: member

ACK 4d4dca43fc591bf8fae7af74670f6e96650ef34b

hebasto approved

hebasto commented at 7:32 AM on April 3, 2022: member

ACK 4d4dca43fc591bf8fae7af74670f6e96650ef34b, tested with reverting changes from bitcoin-core/gui#568, and getting an expected test failure.

shaavan approved

shaavan commented at 1:25 PM on April 3, 2022: contributor

ACK 4d4dca43fc591bf8fae7af74670f6e96650ef34b

The added test seems logically sound and is written in a very easy-to-understand manner.
I was able to verify that:
- The test ran successfully on the PR branch.
- The test failed when the changes done in #568 were reverted.
I also like the comments added along with the changes done in #568. These help the code reader understand the reasoning behind writing the code the way it is written.

hebasto merged this on Apr 4, 2022

hebasto closed this on Apr 4, 2022

vasild deleted the branch on Apr 4, 2022

sidhujag referenced this in commit 3417991add on Apr 4, 2022

bitcoin-core locked this on Apr 4, 2023

hebasto commented at 5:03 PM on February 24, 2024: member

Unfortunately, the test_bitcoin-qt.exe fails on Windows (v26.1rc1):

FAIL!  : OptionTests::parametersInteraction() 'gArgs.IsArgSet("-listen")' returned FALSE.

bitcoin-core unlocked this on Feb 26, 2024

jamesdorfman referenced this in commit 7bc62ad48b on Jul 8, 2024

kwvg referenced this in commit 63c747c566 on Jan 27, 2025

kwvg referenced this in commit afc41c1d64 on Jan 27, 2025

kwvg referenced this in commit ff2566f3ef on Jan 27, 2025

kwvg referenced this in commit aec29276e7 on Feb 5, 2025

PastaPastaPasta referenced this in commit 96dd1265fc on Feb 6, 2025

bitcoin-core locked this on Feb 25, 2025