Skip to content

test: Set organization name#803

Merged
hebasto merged 2 commits intobitcoin-core:masterfrom
hebasto:240305-appname
Mar 7, 2024
Merged

test: Set organization name#803
hebasto merged 2 commits intobitcoin-core:masterfrom
hebasto:240305-appname

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Mar 5, 2024

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.

@hebasto
qt, test: Set organization name
49cf635 
If `setOrganizationName()` and `setApplicationName()` has not been
previously called, the `QSettings` object will not be able to read or
write any settings.
@hebasto hebasto added the Tests label Mar 5, 2024
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 5, 2024 •
edited
Loading

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.

@hebasto
Copy link
Member Author

hebasto commented Mar 5, 2024

Friendly ping @ryanofsky @sipsorcery @furszy @jarolrod

@hebasto
qt, test: Clean settings after tests
0dcbad3 
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.
@hebasto hebasto mentioned this pull request Mar 5, 2024
Merged
pablomartin4btc
pablomartin4btc reviewed Mar 5, 2024
View reviewed changes
Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 0dcbad3

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.

@sipsorcery
Copy link
Contributor

sipsorcery commented Mar 5, 2024

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

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

Does that demonstrate the fix?

@hebasto
Copy link
Member Author

hebasto commented Mar 5, 2024

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

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

Does that demonstrate the fix?

Exactly.

@sipsorcery
Copy link
Contributor

sipsorcery commented Mar 5, 2024

tACK 49cf635.

@DrahtBot DrahtBot requested a review from sipsorcery March 5, 2024 21:44
@hebasto hebasto merged commit c2c6a7d into bitcoin-core:master Mar 7, 2024
@hebasto hebasto deleted the 240305-appname branch March 7, 2024 12:51
@hebasto hebasto mentioned this pull request Mar 8, 2024
Closed
ryanofsky
ryanofsky reviewed Mar 11, 2024
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-merge code review ACK 0dcbad3

src/qt/test/test_main.cpp
AppTests app_tests(app);
num_test_failures += QTest::qExec(&app_tests);
{
BitcoinApplication app;
Copy link
Contributor

@ryanofsky ryanofsky Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "qt, test: Clean settings after tests" (0dcbad3)

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.

Copy link
Member Author

@hebasto hebasto Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a required change because ~BitcoinApplication calls ~BitcoinGUI, which in turn saves the window geometry to the settings.

@bitcoin-core bitcoin-core locked and limited conversation to collaborators Mar 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky left review comments

@sipsorcery sipsorcery Awaiting requested review from sipsorcery

+1 more reviewer

@pablomartin4btc pablomartin4btc pablomartin4btc left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test: migrateSettings fails on Windows

5 participants

@hebasto @DrahtBot @sipsorcery @ryanofsky @pablomartin4btc