FdoSecrets: Add unit tests for secret service integration

[Aetf]

Type of change

• ✅ Refactor (significant modification to existing code)
• ✅ New feature (non-breaking change which adds functionality)

Description and Context

This adds proper unit tests to the secret service integration plugin. The tests cover most exposed API and some regression tests for previously fixed issues.

The first commit adds all test cases. The whole test is added as a GUI test because currently the plugin still depends on DatabaseWidget. Other changes included in this commit:

• The ownership management of DatabaseOpenDialog inside DatabaseTabWidget is changed from using QScopedPointer to setting QObject parent. The lifetime of DatabaseOpenDialog is not affected, but it enables the testing code to access the DatabaseOpenDialog via object hierarchy.
• Added a new argument LAUNCHER to the add_unit_test macro. The calling site can specify a launcher program used to start the test binary. This is needed to use dbus-run-session to run the dbus test.

The following commits fix issues discovered by newly added tests. One notable change is the addition of skipProtected option to the entry searcher. The option is disabled by default, so the behavior remains the same as before. When enabled, search terms referring to password or protected attributes are filtered out before the searcher performs the match. And if there are no terms left after filtering, the default is changed to not match, so the search API via DBus won't return everything when passing only this kind of terms. This is a more general solution of #4008, which only considers passwords.

Testing strategy

make test

CTest runner will automatically use dbus-run-session to run testguifdosecrets.

Checklist:

• ✅ I have read the CONTRIBUTING document. [REQUIRED]
• ✅ My code follows the code style of this project. [REQUIRED]
• ✅ All new and existing tests passed. [REQUIRED]
• ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
• ✅ My change requires a change to the documentation, and I have updated it accordingly.
• ✅ I have added tests to cover my changes.

[Aetf]

Enough fighting CI for today 🤦‍♂️. The last issue seems to be missing dbus-run-session in the CI environment.

[droidmonkey]

Easy fix, we can add that. Are there any version constraints?

[Aetf]

Just did a quick search, on ubuntu, dbus-run-session is provided by the dbus package. I think the current 1.10.6 available in ubuntu repo should be fine, although I've only tested with 1.12.6.

[Aetf]

Ping. I rebased this on the latest develop.

[droidmonkey]

If the tests pass I'll merge it :-D

[Aetf]

Cool! Thanks for reviewing. This depends on #4776 to correctly cleanup between test cases, though.

[droidmonkey]

Merged that one, please rebase again

[Aetf]

The failure in CI seems unrelated in TestCli::testEdit (Windows) and TestCli::testYubiKeyOption (Ubuntu).

[droidmonkey]

Yah sometimes those trip for random reasons

[lkilo]

🐈

…

On Mon, May 25, 2020, 8:03 PM Aetf ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/gui/DatabaseTabWidget.cpp <#4232 (comment)> : > @@ -49,7 +49,7 @@ DatabaseTabWidget::DatabaseTabWidget(QWidget* parent) : QTabWidget(parent) , m_dbWidgetStateSync(new DatabaseWidgetStateSync(this)) , m_dbWidgetPendingLock(nullptr) - , m_databaseOpenDialog(new DatabaseOpenDialog()) + , m_databaseOpenDialog(new DatabaseOpenDialog(this)) Because otherwise, the testing code has no way to access the database open dialog. And changing from using a QScopedPointer to using QObject parent should not change the lifetime of DatabaseOpenDialog in any non-negligible way. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4232 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFUITJITVFGFLHGCZTPZTE3RTKXINANCNFSM4KLK36NQ> .

[droidmonkey]

I made some minor tweaks (see comments above), good to go now.

[Aetf]

Oh thanks, I was going to address the rest comments tomorrow :P.

All looks good to me except the logic behind skipping protected in the entry searcher. See my inline comments. Basically, pre-filtering the protected terms is still necessary and I think your initial comment of putting it in parsing should work. I just didn't get time to sit down and actually look at it. Will do it tomorrow.

[Aetf]

Rebased and squashed the fixups into the original commit.

[Aetf]

It's TestCli::testYubiKeyOption again failing on Windows :/