test: add regression test for #567 #569

vasild · 2022-03-24T17:34:27Z

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

Also, add a comment and dedup a long expression.

vasild · 2022-03-24T17:42:26Z

How to test this:

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

don't click here

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

PASS   : OptionTests::parametersInteraction()

revert the fix from options: flip listenonion to false if not listening #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)]

jonatack · 2022-03-24T19:59:03Z

Concept ACK

vasild · 2022-03-25T09:55:43Z

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.

😕

ryanofsky · 2022-03-30T17:28:53Z

Thank you for following up with this! 🥳

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"):

gui/src/init.cpp

Lines 828 to 829 in f4e5d70

    
           if (!fs::is_directory(gArgs.GetBlocksDirPath())) { 
        
               return InitError(strprintf(_("Specified blocks directory \"%s\" does not exist."), args.GetArg("-blocksdir", "")));

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).

jonatack

ACK cb018e7 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");
+    }

src/qt/optionsmodel.cpp

jonatack · 2022-03-30T20:34:48Z

src/qt/test/optiontests.cpp

+    gArgs.LockSettings([&](util::Settings& s) {
+        s.forced_settings.erase("listen");
+        s.forced_settings.erase("listenonion");
+    });


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"); }

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.

A followup to bitcoin-core#568 Co-authored-by: Jon Atack <[email protected]>

vasild · 2022-03-31T13:40:25Z

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 · 2022-03-31T14:42:26Z

ACK cae12fc provided the CI is happy

jarolrod

ACK cae12fc

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

src/qt/test/optiontests.cpp

vasild · 2022-04-01T08:33:40Z

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

Invalidates ACKs from @jonatack and @jarolrod

jarolrod · 2022-04-01T14:59:53Z

reACK 4d4dca4

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

jonatack

ACK 4d4dca4

hebasto

ACK 4d4dca4, tested with reverting changes from #568, and getting an expected test failure.

shaavan

ACK 4d4dca4

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 options: flip listenonion to false if not listening #568 were reverted.
I also like the comments added along with the changes done in options: flip listenonion to false if not listening #568. These help the code reader understand the reasoning behind writing the code the way it is written.

hebasto · 2024-02-24T17:03:15Z

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

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

vasild requested review from ryanofsky and jonatack March 24, 2022 17:35

vasild mentioned this pull request Mar 24, 2022

options: flip listenonion to false if not listening #568

Merged

jonatack reviewed Mar 30, 2022

View reviewed changes

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

3b82608

A followup to bitcoin-core#568 Co-authored-by: Jon Atack <[email protected]>

vasild force-pushed the options_test branch from cb018e7 to cae12fc Compare March 31, 2022 13:35

jarolrod reviewed Apr 1, 2022

View reviewed changes

src/qt/test/optiontests.cpp Show resolved Hide resolved

test: add regression test for bitcoin-core/issues/567

4d4dca4

vasild force-pushed the options_test branch from cae12fc to 4d4dca4 Compare April 1, 2022 08:32

jarolrod added the Tests label Apr 1, 2022

jonatack reviewed Apr 2, 2022

View reviewed changes

hebasto approved these changes Apr 3, 2022

View reviewed changes

shaavan approved these changes Apr 3, 2022

View reviewed changes

hebasto merged commit 47bac47 into bitcoin-core:master Apr 4, 2022

vasild deleted the options_test branch April 4, 2022 14:24

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 4, 2022

Merge bitcoin-core/gui#569: test: add regression test for bitcoin#567

3417991

ryanofsky mentioned this pull request Apr 5, 2022

interfaces: Expose settings.json methods to GUI bitcoin/bitcoin#15936

Merged

bitcoin-core locked and limited conversation to collaborators Apr 4, 2023

bitcoin-core unlocked this conversation Feb 26, 2024

jamesdorfman added a commit to jamesdorfman/elements that referenced this pull request Jul 8, 2024

Merge 47bac47 into merged_master (Bitcoin PR bitcoin-core/gui#569)

7bc62ad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add regression test for #567 #569

test: add regression test for #567 #569

vasild commented Mar 24, 2022

vasild commented Mar 24, 2022

jonatack commented Mar 24, 2022

vasild commented Mar 25, 2022 •

edited

Loading

ryanofsky commented Mar 30, 2022

jonatack left a comment

jonatack Mar 30, 2022 •

edited

Loading

vasild Mar 31, 2022

vasild commented Mar 31, 2022

jonatack commented Mar 31, 2022

jarolrod left a comment

vasild commented Apr 1, 2022 •

edited

Loading

jarolrod commented Apr 1, 2022

jonatack left a comment

hebasto left a comment

shaavan left a comment

hebasto commented Feb 24, 2024

test: add regression test for #567 #569

test: add regression test for #567 #569

Conversation

vasild commented Mar 24, 2022

vasild commented Mar 24, 2022

jonatack commented Mar 24, 2022

vasild commented Mar 25, 2022 • edited Loading

ryanofsky commented Mar 30, 2022

jonatack left a comment

Choose a reason for hiding this comment

jonatack Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

vasild Mar 31, 2022

Choose a reason for hiding this comment

vasild commented Mar 31, 2022

jonatack commented Mar 31, 2022

jarolrod left a comment

Choose a reason for hiding this comment

vasild commented Apr 1, 2022 • edited Loading

jarolrod commented Apr 1, 2022

jonatack left a comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

hebasto commented Feb 24, 2024

vasild commented Mar 25, 2022 •

edited

Loading

jonatack Mar 30, 2022 •

edited

Loading

vasild commented Apr 1, 2022 •

edited

Loading