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.

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

Create QML test automation target (qml/test/test_bitcoin-qt) #215

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

johnny9
Copy link
Contributor

@johnny9 johnny9 commented Jan 13, 2023

Adds src/qml/test/test_bitcoin-qt as a new test target. This program is built using the QuickTest library. It can be used to validate our qml code by rederimg our components and providing simulated user input. It is set to only be built if QuickTest library is found, gui tests are enabled, and bitcoin-qt is configured for qml.

As a part of test setup, a test implementation of our imageprovider is added to the test target to avoid bringing in too many dependencies. Additionally, a fake AppMode component is added to mock setting the device layout mode.

More information about the QuickTest library can be found at https://doc.qt.io/qt-5.15/qtquicktest-index.html.

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Concept ACK, a good start. Testing that we can progress through onboarding is a great first test case. Will look into failures.

@hebasto
Copy link
Member

hebasto commented Jan 14, 2023

@johnny9

Thank you very much for working on tests!

Mind adding some words to the PR description?

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Concept ACK.

If there are no reasons against it, maybe rename s/tst_onboarding.qml/test_onboarding.qml/?

src/qml/test/tst_onboarding.qml Outdated Show resolved Hide resolved
src/Makefile.qmltest.include Outdated Show resolved Hide resolved
src/qml/test/onboardingtests.h Outdated Show resolved Hide resolved
src/qml/test/org/bitcoincore/qt/AppMode.qml Show resolved Hide resolved
@johnny9
Copy link
Contributor Author

johnny9 commented Jan 14, 2023

tst_*.qml is actually the pattern the library uses to find test cases in a directory.

@johnny9
Copy link
Contributor Author

johnny9 commented Jan 14, 2023

Still have some linking issues with Makefile.qmltest.include it seems as CI isn't happy. I likely linked too make things into the binary.

@hebasto
Copy link
Member

hebasto commented Jan 14, 2023

Still have some linking issues with Makefile.qmltest.include it seems as CI isn't happy. I likely linked too make things into the binary.

Looking into them...

@hebasto
Copy link
Member

hebasto commented Jan 14, 2023

Static builds, which all builds with depends are, require importing of plugins like this:

--- a/src/qml/test/onboardingtests.cpp
+++ b/src/qml/test/onboardingtests.cpp
@@ -1,11 +1,32 @@
 // Copyright (c) 2023 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <qml/test/onboardingtests.h>
+
+#if defined(HAVE_CONFIG_H)
+#include <config/bitcoin-config.h>
+#endif
+
 #include <QtQuickTest>
 #include <QQmlEngine>
 #include <QQmlContext>
 #include <qml/test/testimageprovider.h>
-#include <qml/test/onboardingtests.h>
+
+#if defined(QT_STATICPLUGIN)
+#include <QtPlugin>
+Q_IMPORT_PLUGIN(QtQuick2Plugin)
+Q_IMPORT_PLUGIN(QTestQmlModule)
+#if defined(QT_QPA_PLATFORM_XCB)
+Q_IMPORT_PLUGIN(QXcbIntegrationPlugin);
+#elif defined(QT_QPA_PLATFORM_WINDOWS)
+Q_IMPORT_PLUGIN(QWindowsIntegrationPlugin);
+#elif defined(QT_QPA_PLATFORM_COCOA)
+Q_IMPORT_PLUGIN(QCocoaIntegrationPlugin);
+#elif defined(QT_QPA_PLATFORM_ANDROID)
+Q_IMPORT_PLUGIN(QAndroidPlatformIntegrationPlugin)
+#endif
+#endif
 
 Setup::Setup()
 {

And add to build-aux/m4/bitcoin_qt.m4 a few lines QT_LIBS="$QT_LIBS -L$qt_plugin_path/../qml/QtTest" and _BITCOIN_QT_CHECK_STATIC_PLUGIN([QTestQmlModule], [-lqmltestplugin]) (did not test yet).

@hebasto
Copy link
Member

hebasto commented Jan 14, 2023

Maybe, label this PR as a draft for now?

build-aux/m4/bitcoin_qt.m4 Outdated Show resolved Hide resolved
@hebasto
Copy link
Member

hebasto commented Jan 14, 2023

FWIW, I'm testing this PR on Ubuntu 22.04 with depends:

$ make -C depends NO_WALLET=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 NO_QR=1
$ ./autogen.sh
$ ./configure --disable-fuzz-binary --disable-bench CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site
$ make clean
$ make

@hebasto
Copy link
Member

hebasto commented Jan 14, 2023

From https://doc.qt.io/qt-5/qtquicktest-index.html:

Note: There is no binary compatibility guarantee for the Qt Quick Test module. This means that an application that uses Qt Quick Test is only guaranteed to work with the Qt version it was developed against. However, source compatibility is guaranteed.

Should we skip Quick test when building with depends?

@johnny9 johnny9 marked this pull request as draft January 14, 2023 16:50
@johnny9
Copy link
Contributor Author

johnny9 commented Jan 14, 2023

Thank you for the review Hebasto. Very helpful. Marked as a draft and will unmark after addressing the issues you've mentioned and am more confident it can get through CI.

@hebasto
Copy link
Member

hebasto commented Jan 15, 2023

@johnny9

Here is a quick and dirty branch which is built successfully in all CI tasks. However, running qml/test/test_bitcoin-qt in the CI environment needs more investigation regarding which value of the QT_QPA_PLATFORM variable should be used.

Feel free to use my branch at your discretion.

@johnny9
Copy link
Contributor Author

johnny9 commented Jan 15, 2023

@johnny9

Here is a quick and dirty branch which is built successfully in all CI tasks. However, running qml/test/test_bitcoin-qt in the CI environment needs more investigation regarding which value of the QT_QPA_PLATFORM variable should be used.

Feel free to use my branch at your discretion.

Thank you. I'll take a look at it and merge it with my branch. I made an attempt last night but struggled with the QT STATIC PLUGIN checks. I'll be investigating how to get this supported in cirrus afterwards. I have some ideas but it'll depend on what kind of platform support they have for a buffer to render into. I'd like to use this to validate the qml in our PRs and maybe even generate screenshots since that eats up a lot of review time.

@hebasto
Copy link
Member

hebasto commented Jan 15, 2023

I made an attempt last night but struggled with the QT STATIC PLUGIN checks.

Still thinking it doesn't worth. See #215 (comment).

@johnny9 johnny9 force-pushed the qmltest branch 3 times, most recently from 5cd0be3 to 38a8d16 Compare January 16, 2023 13:06
@hebasto
Copy link
Member

hebasto commented Jan 16, 2023

The qml/pages/onboarding/OnboardingWizard.qml is still to be added to QML_RES_QML.

@johnny9 johnny9 force-pushed the qmltest branch 5 times, most recently from 0f0cfe3 to 75da2a2 Compare January 17, 2023 01:07
@johnny9 johnny9 marked this pull request as ready for review January 17, 2023 17:16
@hebasto
Copy link
Member

hebasto commented Jan 20, 2023

Might the introducing of src/qml/pages/onboarding/OnboardingWizard.qml be split up into its own PR?

@johnny9
Copy link
Contributor Author

johnny9 commented Sep 1, 2023

Update from 75da2a2 to ed8222b

  • Rebased with main

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

General Concept ACK

Test cases work and run

********* Start testing of onboarding *********
Config: Using QtTest library 5.15.10, Qt 5.15.10 (arm64-little_endian-lp64 shared (dynamic) release build; by Clang 14.0.0 (clang-1400.0.29.202) (Apple)), osx 13.5
PASS   : onboarding::initTestCase()
PASS   : onboarding::test_all_continue_buttons()
PASS   : onboarding::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1042ms
********* Finished testing of onboarding *********

@@ -0,0 +1,110 @@
// Copyright (c) 2023 The Bitcoin Core developers
Copy link
Member

Choose a reason for hiding this comment

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

There has to be a way for us to not have to copy over this into qml/test


var cover = findChild(onboardingWizard, "onboardingCover")
verify(cover, onboardingWizard.currentItem)
mouseClick(findChild(cover, "continueButton"))
Copy link
Member

Choose a reason for hiding this comment

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

What about abstracting a lot of the repetitive action of finding and clicking into a helper function?

helper function looking something like:

function clickContinueAndVerify(pageId, expectedIndex) {
    var page = findChild(onboardingWizard, pageId)
    verify(page, onboardingWizard.currentItem)
    mouseClick(findChild(page, "continueButton"))
    verify(onboardingWizard.currentIndex == expectedIndex)
}

Then calling like:

clickContinueAndVerify("onboardingCover", 1)
wait(200)
clickContinueAndVerify("onboardingStrengthen", 2)

Copy link
Member

Choose a reason for hiding this comment

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

A lot of tests would involve a repetitive function, so good to think about this now to establish a style for how we write our tests

@jarolrod
Copy link
Member

jarolrod commented Sep 8, 2023

Need to cleanup the commit history :D

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.

Concept ACK

It would be good to have this soon.

tested ed8222b on Ubuntu 22.04.

********* Start testing of onboarding *********
Config: Using QtTest library 5.15.3, Qt 5.15.3 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 11.3.0), ubuntu 22.04
PASS   : onboarding::initTestCase()
PASS   : onboarding::test_all_continue_buttons()
PASS   : onboarding::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1049ms
********* Finished testing of onboarding *********

Apart from @jarolrod's comments, since we are moving away from SwipeView I think we'd need to update this to PageStacks (#428).

At least in Ubuntu, I see the UI gets opened and pages are switching, not sure if this is on purpose and can be hidden (I haven't checked the code) as in current Qt this is not happening.

I got these errors before the successful results pasted above.
/src/qml/test/test_bitcoin-qt 
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/controls/Theme.qml:13:5: QML Settings: Failed to initialize QSettings instance. Status code is: 1
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/controls/Theme.qml:13:5: QML Settings: The following application identifiers have not been set: QVector("organizationName", "organizationDomain")
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/StorageOptions.qml:33: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/AboutOptions.qml:54: ReferenceError: nodeModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/DeveloperOptions.qml:20: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/DeveloperOptions.qml:45: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/DeveloperOptions.qml:16: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/DeveloperOptions.qml:41: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/StorageOptions.qml:27: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/StorageOptions.qml:25: ReferenceError: chainModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/StorageOptions.qml:42: ReferenceError: chainModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/StorageOptions.qml:41: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/StorageSettings.qml:19: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/StorageSettings.qml:43: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/ConnectionSettings.qml:17: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/ConnectionSettings.qml:30: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/ConnectionSettings.qml:43: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/components/ConnectionSettings.qml:56: ReferenceError: optionsModel is not defined
file:///home/pablo/workspace/bitcoin-core-gui-qml/src/qml/pages/onboarding/OnboardingStorageLocation.qml:23: ReferenceError: chainModel is not defined
********* Start testing of onboarding *********
Config: Using QtTest library 5.15.3, Qt 5.15.3 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 11.3.0), ubuntu 22.04
PASS   : onboarding::initTestCase()
PASS   : onboarding::test_all_continue_buttons()
PASS   : onboarding::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1049ms
********* Finished testing of onboarding *********

@pablomartin4btc
Copy link
Contributor

During discussions with @jarolrod about reorganizing the project structure (components, pages, controls, etc.), we noted that tests will need to be updated once Qt 6 is introduced, along with CMake. Considering this, it might be better to treat this as a draft for now, until we decide whether to create a new PR from scratch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants