test: Set organization name #803

1. 
   hebasto commented at 7:11 am on March 5, 2024: member

   From Qt docs:

   If QCoreApplication::setOrganizationName() and QCoreApplication::setApplicationName() has not been previously called, the QSettings object will not be able to read or write any settings, and status() will return AccessError.

   Fixes #799.

2. qt, test: Set organization name

   If `setOrganizationName()` and `setApplicationName()` has not been
   previously called, the `QSettings` object will not be able to read or
   write any settings.

   49cf63522e
3. hebasto added the label Tests on Mar 5, 2024
4. 
   DrahtBot commented at 7:11 am on March 5, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	pablomartin4btc
   Stale ACK	sipsorcery

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

5. 
   hebasto commented at 7:12 am on March 5, 2024: member

   Friendly ping @ryanofsky @sipsorcery @furszy @jarolrod
6. qt, test: Clean settings after tests

   A test suite should not leave any artifacts except for those explicitly
   expected.
   
   This change is easy to review with `git diff --ignore-all-space`
   command.

   0dcbad341b
7. hebasto force-pushed on Mar 5, 2024
8. 
   pablomartin4btc commented at 2:22 pm on March 5, 2024: contributor

   utACK 0dcbad341b0a8420a899c6dce0db56dd0deaa036

   Just for ref: we’ve faced similar issue in the QML project, where we had to set the values above in the stack.

   QML Settings: Failed to initialize QSettings instance. Status code is: 1

   Issue #361 fixed with #360.

9. 
   sipsorcery commented at 9:22 pm on March 5, 2024: member

   Building this PR with msvc and running test_bitcoin-qt.exe gives me:

    0...snip...
    1********* Start testing of OptionTests *********
    2Config: Using QtTest library 5.15.11, Qt 5.15.11 (x86_64-little_endian-llp64 static release build; by MSVC 2022), windows 11
    3PASS   : OptionTests::initTestCase()
    4PASS   : OptionTests::migrateSettings()
    5PASS   : OptionTests::integerGetArgBug()
    6PASS   : OptionTests::parametersInteraction()
    7PASS   : OptionTests::extractFilter()
    8PASS   : OptionTests::cleanupTestCase()
    9Totals: 6 passed, 0 failed, 0 skipped, 0 blacklisted, 20ms
   10********* Finished testing of OptionTests *********
   11...snip...
   

   Does that demonstrate the fix?

10. 
    hebasto commented at 9:42 pm on March 5, 2024: member

    Building this PR with msvc and running test_bitcoin-qt.exe gives me:

     0...snip...
     1********* Start testing of OptionTests *********
     2Config: Using QtTest library 5.15.11, Qt 5.15.11 (x86_64-little_endian-llp64 static release build; by MSVC 2022), windows 11
     3PASS   : OptionTests::initTestCase()
     4PASS   : OptionTests::migrateSettings()
     5PASS   : OptionTests::integerGetArgBug()
     6PASS   : OptionTests::parametersInteraction()
     7PASS   : OptionTests::extractFilter()
     8PASS   : OptionTests::cleanupTestCase()
     9Totals: 6 passed, 0 failed, 0 skipped, 0 blacklisted, 20ms
    10********* Finished testing of OptionTests *********
    11...snip...
    

    Does that demonstrate the fix?

    Exactly.

11. 
    sipsorcery commented at 9:44 pm on March 5, 2024: member

    tACK 49cf63522e202caf326bad161ff6fa05d1076566.
12. DrahtBot requested review from sipsorcery on Mar 5, 2024
13. hebasto merged this on Mar 7, 2024
14. hebasto closed this on Mar 7, 2024

    ---

15. hebasto deleted the branch on Mar 7, 2024
16. in src/qt/test/test_main.cpp:95 in 0dcbad341b

     96     int num_test_failures{0};
     97 
     98-    AppTests app_tests(app);
     99-    num_test_failures += QTest::qExec(&app_tests);
    100+    {
    101+        BitcoinApplication app;
    

    ---

    ---

    

    ryanofsky commented at 3:55 pm on March 11, 2024:

    In commit “qt, test: Clean settings after tests” (0dcbad341b0a8420a899c6dce0db56dd0deaa036)

    It seems reasonable to move the BitcoinApplication app into a nested scope, so BitcoinApplication is fully destroyed before settings.clear() is called. But I don’t understand if this was a required change, or just cleanup, since I would expect the settings.clear() call to work in practice either way.

    ---

    

    hebasto commented at 8:19 pm on March 11, 2024:

    It was a required change because ~BitcoinApplication calls ~BitcoinGUI, which in turn saves the window geometry to the settings.
17. 
    ryanofsky commented at 3:55 pm on March 11, 2024: contributor

    Post-merge code review ACK 0dcbad341b0a8420a899c6dce0db56dd0deaa036

Contributors
hebasto DrahtBot pablomartin4btc sipsorcery ryanofsky

Review Requested
sipsorcery

Labels
Tests