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

Initial Catch2 integration #9199

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

dmaslenko
Copy link
Contributor

@dmaslenko dmaslenko commented Mar 4, 2023

This is a part of migration on Catch2 test framework.
The initial commit includes:

  • New tests2 folder (as temporary name to migrae all tests and then it will be renamed back to the original tests)
  • New CMakeLists.txt files for unit and GUI tests.
  • Unit and GUI tests for PassphraseGenerator and TOTP classes.

Supports #5473

Type of change

  • ✅ Refactor (significant modification to existing code)

@dmaslenko dmaslenko marked this pull request as draft March 4, 2023 07:30
@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch from a84a58b to e8f3a3d Compare March 5, 2023 07:16
@droidmonkey
Copy link
Member

Wow that produces a clean test! Do you think this is worth it to keep going?

@dmaslenko
Copy link
Contributor Author

Yes, definitely we should keep going.

Once I done with both regular and GUI demo tests I will ask you to help with TeamCity to configure the Catch2 installation. Locally I installed it from the git repo as described here but I don't know how to manage the TeamCity.

Once we got working environment and couple demo tests I would love to migrate all the rest tests one by one. Yes, this is a long journey, but I like such kind of job.

For now I have a blocker to run all tests together - new tests does not create a QT application and fails on not initialized app, but standalone run works fine. Probably there is a conflict or the main.cpp is not called, I will debug it...

@phoerious
Copy link
Member

Does this work with all tests? Qt has a bunch of routines that test for conditions repeatedly, executing the event loop in between.

@dmaslenko
Copy link
Contributor Author

Never mind, it works from the command line, something wrong with my CLion.

NewCatch2TestWorksWithAllTests

@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch from e8f3a3d to 8361b22 Compare March 8, 2023 07:24
@dmaslenko
Copy link
Contributor Author

Keep moving, the main window is already displayed, will implement a simple GUI test to check a generated phase on not open DB.

@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch from 8361b22 to 4aa739c Compare March 9, 2023 07:43
@droidmonkey
Copy link
Member

droidmonkey commented Mar 9, 2023

@phoerious qt creates that event loop using the macros at the top of test files. Catch2 bunches all tests into one or more exe's, you define the main function and at that point is when you would establish the qt event loop.

https://stackoverflow.com/a/56569699

@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch 4 times, most recently from 48baab3 to ee2ba64 Compare March 16, 2023 06:08
@dmaslenko dmaslenko marked this pull request as ready for review March 16, 2023 06:09
@dmaslenko
Copy link
Contributor Author

I still have couple ideas to improve tests and add missed test cases for Passphrase Generator but in general it is ready for review.

@droidmonkey
Copy link
Member

You are a machine! This is awesome

@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch from 48d7869 to 738910a Compare March 22, 2023 05:36
@dmaslenko
Copy link
Contributor Author

dmaslenko commented Mar 26, 2023

I am trying to implement new test when an user generated a passphrase and wants to apply it.
For some reason neither Apply nor Close nor x buttons work properly from GUI test. As standalone app it works fine.
See the recorded video from GUI test:
apply and close do not close
For video I put temporary wait() function to have an ability to click the button manually as well.

Any advise where I need to dig deeply?
The apply method is

void PasswordGeneratorWidget::applyPassword()
{
    saveSettings();
    m_passwordGenerated = true;
    emit appliedPassword(m_ui->editNewPassword->text());
    emit closed();
}

where the emit appliedPassword() completed as expected (I can see the changed password in an entry but emit closed() does not close the current Generator widget.

See my the latest code in this test:

SCENARIO_METHOD(FixtureWithDb, "Passphrase Generator on new Entry", "[gui]")

@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch from 738910a to 88b905d Compare March 26, 2023 22:13
@droidmonkey
Copy link
Member

This could happen if you are blocking the event loop, but that would also cause the whole gui to be unresponsive. This can also happen if the button isn't triggering the function you pasted above. Can you set a breakpoint in that function to see if it gets called?

@dmaslenko
Copy link
Contributor Author

I made sure the mentioned function is triggered by a breakpoint and additional console logs.
As I mentioned inside the method

void PasswordGeneratorWidget::applyPassword()
{
    saveSettings();
    m_passwordGenerated = true;
    emit appliedPassword(m_ui->editNewPassword->text());
    emit closed();
}

the first emit works as expected: the password was updated in an entry, the second emit does not close the widget.
I even changed the order of emits, like this

    emit closed();
    emit appliedPassword(m_ui->editNewPassword->text());

the same the first emit close does not work, the second emit appliedPassword works fine but should not because the widget is to be closed.

I think something wrong with this mouse click:

            AND_WHEN("User clicks Apply Password button")
            {
                auto* applyPasswordButton = pPwGenWidget->findChild<QPushButton*>("buttonApply");
                QTest::mouseClick(applyPasswordButton, Qt::MouseButton::LeftButton);
                QApplication::processEvents();

because this variant works fine

            AND_WHEN("User clicks Apply Password button")
            {
//                auto* applyPasswordButton = pPwGenWidget->findChild<QPushButton*>("buttonApply");
//                QTest::mouseClick(applyPasswordButton, Qt::MouseButton::LeftButton);
//                QApplication::processEvents();
                QTest::keyClick(pPassPhraseWidget, Qt::Key_Escape);

but this one does not work

            AND_WHEN("User clicks Apply Password button")
            {
                auto* applyPasswordButton = pPwGenWidget->findChild<QPushButton*>("buttonApply");
                QTest::mouseClick(applyPasswordButton, Qt::MouseButton::LeftButton);
                QApplication::processEvents();
                QTest::keyClick(pPassPhraseWidget, Qt::Key_Escape);

even the latest Esc command does not close the widget.

I am continue the investigation...

@droidmonkey
Copy link
Member

I think you are getting stuck in the process events call

@dmaslenko
Copy link
Contributor Author

The password generator has 3 ways to apply a generated password:

  • Mouse click on the Apply button
  • Ctrl + S
  • Ctrl + Enter

I tried all of them and only this shortcut works fine:

QTest::keyClick(pPassPhraseWidget, Qt::Key::Key_S, Qt::ControlModifier);

The mouse click does not work as mentioned above. BTW, I found a similar Stackoverflow question.

The second shortcut does not work as well:

QTest::keyClick(pPassPhraseWidget, Qt::Key::Key_Enter, Qt::ControlModifier);

All of these 3 ways work fine in a standalone app.
It can be a Linux specific issue, I will try to run on MacOS...

@dmaslenko

This comment was marked as resolved.

@droidmonkey droidmonkey added this to the v2.8.0 milestone Apr 4, 2023
@droidmonkey droidmonkey marked this pull request as draft April 4, 2023 03:25
@droidmonkey droidmonkey force-pushed the test/initial_catch2 branch from 88b905d to 6ad2fbb Compare April 4, 2023 04:01
@droidmonkey
Copy link
Member

I fixed a crash you had in the fixtureWithDb class. I can replicate the issue with the password generator dialog, but it might be because of sequencing in the test case itself. You should add a wait condition to the "isVisible" check.

@dmaslenko
Copy link
Contributor Author

Thank you, you really helped and unblocked me with the crash.
I recognized this place, originally it was as in TestGUI.cpp: fileDialog()->setNextFileName(m_dbFilePath); but I had some issue and fixed it by deleting of the returned pointer. I don't remember exactly the problem. Anyway, let's keep the fix and I will pay attention if run into an issue again.
However, the issue with clicking on the Apply or Close buttons is still open, a wait(10000) does not help, continue the investigation...

@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch 2 times, most recently from b9c52e9 to 8b0fec2 Compare February 18, 2024 02:40
@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch 2 times, most recently from 6de6eae to b4170fa Compare March 2, 2024 05:00
@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch from 3f59187 to 14d9ba9 Compare March 15, 2024 15:31
dmaslenko and others added 23 commits April 5, 2024 20:34
Now the PassphraseGenerator class is covered by 98%.
[Why]
  With the current implementation it is impossible to get m_wordCount = 0.
This fix is needed for GUI scenario implemented in a single test class.
In this case when one scenario is completed and the MainWindow destroyed
the next scenario creates another new MainWindow instance but fails on
already existent KeeShare instance.
Added new tests, the coverage increased from 50 to 96%.
Added 2 methods will be reused in the following commits.
@dmaslenko dmaslenko force-pushed the test/initial_catch2 branch from 14d9ba9 to f2a7dae Compare April 6, 2024 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: refactoring Pull request that refactors code pr: tests Pull requests that adds or modifies tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants