Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: Set organization name #803

Merged
merged 2 commits into from Mar 7, 2024
Merged

test: Set organization name #803

merged 2 commits into from Mar 7, 2024

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

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
Member

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
Member

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
16 checks passed
@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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanofsky ryanofsky ryanofsky left review comments

@pablomartin4btc pablomartin4btc pablomartin4btc left review comments

@sipsorcery sipsorcery Awaiting requested review from sipsorcery

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