test: Set organization name #803

pull hebasto wants to merge 2 commits into bitcoin-core:master from hebasto:240305-appname changing 2 files +41 −19
  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
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 23:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me