test: add regression test for #567 by vasild · Pull Request #569 · bitcoin-core/gui

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 #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 · 2024-02-24T17:03:15Z

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

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

, merge bitcoin-core/gui#408, #398, #434, #439, #319, #404, #569, #576, #618, #620, #631, #591, partial bitcoin#23757 (qt backports: part 3) 0cb2724 merge bitcoin-core/gui#591: Add tests for `tableView` in `AddressBookPage` dialog (Kittywhiskers Van Gogh) 6bf4854 partial bitcoin#23757: fix GUI not loading on Qt 5.15 (Kittywhiskers Van Gogh) 75a1016 merge bitcoin-core/gui#631: Disallow encryption of watchonly wallets (Kittywhiskers Van Gogh) 18d1523 merge bitcoin-core/gui#620: Replace `QRegExp` with `QRegularExpression` (Kittywhiskers Van Gogh) 6e4eee0 merge bitcoin-core/gui#618: Add `transactionoverviewwidget.cpp` source file (Kittywhiskers Van Gogh) b25f165 merge bitcoin-core/gui#576: Add qt unit test runner summary (Kittywhiskers Van Gogh) aec2927 merge bitcoin-core/gui#569: add regression test for bitcoin-core/gui#567 (Kittywhiskers Van Gogh) f9b7614 merge bitcoin#24498: Avoid crash on startup if int specified in settings.json (Kittywhiskers Van Gogh) 40b09dd merge bitcoin#24375: Do not use `LocalTestingSetup` in getarg_tests test file (Kittywhiskers Van Gogh) 817a95a merge bitcoin#24041: Restore GetIntArg saturating behavior (Kittywhiskers Van Gogh) d451246 merge bitcoin-core/gui#404: Fix various edge case bugs in QValidatedLineEdit (Kittywhiskers Van Gogh) c02483c merge bitcoin-core/gui#319: Paste button in Open URI dialog (Kittywhiskers Van Gogh) 3db335f merge bitcoin-core/gui#439: Do not show unused widgets at startup (Kittywhiskers Van Gogh) 33da874 merge bitcoin-core/gui#434: Keep InitExecutor in main gui thread (Kittywhiskers Van Gogh) 3f9dca5 merge bitcoin-core/gui#398: Pass WalletModel object to the WalletView constructor (Kittywhiskers Van Gogh) 9e58f8c merge bitcoin-core/gui#408: Add missing mnemonics in menu bar options (Kittywhiskers Van Gogh) Pull request description: ## Additional Information | `develop` (0972dfe) | This PR | | ------------------------------------------------------------ | ------------------------------------------------------------ | | ![image](https://github.com/user-attachments/assets/0221e53f-1a96-4746-8438-af0655bc62e4) | ![image](https://github.com/user-attachments/assets/992ed506-e3d7-4131-85d8-c433fbaa837b) | | ![image](https://github.com/user-attachments/assets/cdff222c-7312-49b4-9ff1-d7bb5c785290) | ![image](https://github.com/user-attachments/assets/0e07fcb1-4f56-4511-bacc-e249906e39e5) | ## Breaking Changes * The menu bar mnemonic (highlighted in **bold**) for "Open wallet **c**onfiguration file" has been reassigned to "Load PSBT from **c**lipboard…". The replacement mnemonic is "Open **w**allet configuration file". ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: re-utACK 0cb2724 PastaPastaPasta: utACK 0cb2724 Tree-SHA512: 8039d4a8676b3f680f3aab63a22e2412794a7744440139111377915891597c98d1a68d9ceccecec4afb3e87fff4e1a023565f469de0f205cf764b9666342ccd1

vasild requested review from jonatack and ryanofsky 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

kwvg added a commit to kwvg/dash that referenced this pull request Feb 5, 2025

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

aec2927

bitcoin-core locked and limited conversation to collaborators Feb 25, 2025

Conversation

vasild commented Mar 24, 2022

Uh oh!

vasild commented Mar 24, 2022

Uh oh!

jonatack commented Mar 24, 2022

Uh oh!

vasild commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 30, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

vasild commented Mar 31, 2022

Uh oh!

jonatack commented Mar 31, 2022

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod commented Apr 1, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Feb 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vasild commented Mar 25, 2022 •

edited

Loading

jonatack Mar 30, 2022 •

edited

Loading

vasild commented Apr 1, 2022 •

edited

Loading