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

Improve test coverage #2351

Merged
merged 6 commits into from Oct 19, 2018

Conversation

Projects
None yet
3 participants
@phoerious
Copy link
Member

phoerious commented Oct 3, 2018

Description

This PR updates the coverage report generation target, replaces lcov with gcov and adds extensive unit tests to cover the CLI module. It also fixes a bug I discovered during unit test development, where the export command would just dump the raw XML body without decrypting embedded Salsa20-protected values first, making the export basically useless, since passwords are scrambled.

In addition, all uses of qCritical() with untranslatble raw char* sequences were removed in favor of proper locale strings. These are written to STDERR through QTextStreams and support output redirection for testing purposes. With this change, error messages don't depend on the global Qt logging settings and targets anymore and go directly to the terminal or into a file if needed.

Lastly, all CLI commands received a dedicated -h/--help option.

Motivation and context

Generation of unit test coverage reports used to be quite complicated and required a lot of different settings, including a custom CMake build type. This patch updates the coverage CMake module to only require -DWITH_COVERAGE=ON to be set on a normal Debug build in order to create a coverage target.

This patch also moves away from lcov in favor of gcovr, since lcov appears to be broken in GCC 8. However, the routines for generating lcov reports still exist, so provided lcov receives updates and there is sufficient reason to switch back, it is easy to do so.

How has this been tested?

Tests pass and increase coverage by about 5%.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

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.

@phoerious phoerious added this to the v2.4.0 milestone Oct 3, 2018

@phoerious phoerious requested a review from droidmonkey Oct 3, 2018

@phoerious phoerious force-pushed the feature/coverage branch 6 times, most recently from dab0949 to 89e5734 Oct 3, 2018

@phoerious phoerious changed the title Feature/coverage Improve test coverage Oct 3, 2018

@phoerious phoerious force-pushed the feature/coverage branch 7 times, most recently from 64a8c89 to 77d9d2c Oct 3, 2018

@droidmonkey
Copy link
Member

droidmonkey left a comment

The new bootstrap namespace is a great addition!

Show resolved Hide resolved src/cli/Utils.cpp
if (!Crypto::init()) {
qFatal("Fatal error while testing the cryptographic functions:\n%s", qPrintable(Crypto::errorString()));
return EXIT_FAILURE;
}

QCoreApplication app(argc, argv);
app.setApplicationVersion(KEEPASSX_VERSION);
QCoreApplication::setApplicationVersion(KEEPASSX_VERSION);

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 4, 2018

Member

This might be a good time to change this to KEEPASSXC_VERSION

include(CLangFormat)

if(UNIX AND NOT APPLE)
find_package(Qt5 COMPONENTS Core Network Concurrent Widgets Test LinguistTools DBus REQUIRED)
find_package(Qt5 COMPONENTS Core Network Concurrent Gui Widgets Test LinguistTools DBus REQUIRED)

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 4, 2018

Member

Can we instead put these components in a list and use that for each find_package?

return true;
}
}
bool hasChild(const QObject* parent, const QObject* child)

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 4, 2018

Member

Shouldn't we just be using QObject::findChild(name) and passing in child->objectName?

http://doc.qt.io/qt-5/qobject.html#findChild

A quick look at the usages of this function and it seems to be misused. For example, it is used to check if the recycle bin contains an entry. That is inappropriate as you should use the Group function "findEntryByUUID" instead of relying on the pointer value of the entry. See cli/Remove.cpp line 87.

This comment has been minimized.

Copy link
@phoerious

phoerious Oct 4, 2018

Author Member

Probably. I just copied it over.

Show resolved Hide resolved src/core/Tools.cpp
Show resolved Hide resolved src/format/KdbxReader.cpp
Show resolved Hide resolved src/format/KeePass2Reader.cpp
@@ -180,7 +183,7 @@ add_unit_test(NAME testrandom SOURCES TestRandom.cpp
add_unit_test(NAME testentrysearcher SOURCES TestEntrySearcher.cpp
LIBS ${TEST_LIBRARIES})

add_unit_test(NAME testcsvexporter SOURCES TestCsvExporter.cpp
add_unit_test(NAME testcsveporter SOURCES TestCsvExporter.cpp

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 4, 2018

Member

typo

vi-n added a commit to vi-n/keepassxc that referenced this pull request Oct 4, 2018

Update code with changes made in in keepassxreboot#2351
In pull request keepassxreboot#2351 phoerious updated the coding style for cli
components. This commit implements similar changes for the new Create
component

@phoerious phoerious force-pushed the feature/coverage branch 3 times, most recently from 5d0f827 to b82ea87 Oct 4, 2018

@phoerious phoerious force-pushed the feature/coverage branch 2 times, most recently from b3c90e9 to 90c508d Oct 19, 2018

@keepassxreboot keepassxreboot deleted a comment from phoerious Oct 19, 2018

Update and fix test coverage report generation
Generation of unit test coverage reports used to be quite complicated
and required a lot of different settings, including a custom CMake
build type. This patch updates the coverage CMake module to only
require -DWITH_COVERAGE=ON to be set on a normal Debug build in order
to create a coverage target.

This patch also moves away from lcov in favor of gcovr, since lcov appears
to be broken in GCC 8. However, the routines for generating lcov reports
still exist, so provided lcov receives updates and there is sufficient
reason to switch back, it is easy to do so.

phoerious added some commits Sep 29, 2018

Add CLI tests and improve coding style and i18n
The CLI module was lacking unit test coverage and showed some severe
coding style violations, which this patch addresses.

In addition, all uses of qCritical() with untranslatble raw char*
sequences were removed in favor of proper locale strings. These are
written to STDERR through QTextStreams and support output
redirection for testing purposes. With this change, error messages don't
depend on the global Qt logging settings and targets anymore and go
directly to the terminal or into a file if needed.

This patch also fixes a bug discovered during unit test development,
where the extract command would just dump the raw XML contents without
decrypting embedded Salsa20-protected values first, making the XML
export mostly useless, since passwords are scrambled.

Lastly, all CLI commands received a dedicated -h/--help option.

@phoerious phoerious force-pushed the feature/coverage branch from 8914d0a to 77adbef Oct 19, 2018

@droidmonkey droidmonkey merged commit c749f70 into develop Oct 19, 2018

3 checks passed

CodeFactor 6 issues fixed. 3 issues found.
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@droidmonkey droidmonkey deleted the feature/coverage branch Oct 19, 2018

vi-n added a commit to vi-n/keepassxc that referenced this pull request Nov 13, 2018

Update code with changes made in in keepassxreboot#2351
In pull request keepassxreboot#2351 phoerious updated the coding style for cli
components. This commit implements similar changes for the new Create
component

vi-n added a commit to vi-n/keepassxc that referenced this pull request Nov 13, 2018

Update code with changes made in in keepassxreboot#2351
In pull request keepassxreboot#2351 phoerious updated the coding style for cli
components. This commit implements similar changes for the new Create
component
@darix

This comment has been minimized.

Copy link

darix commented on src/crypto/Crypto.cpp in 18b2283 Nov 26, 2018

This change might be an accident or is this really needed for the coverage change?

This comment has been minimized.

Copy link
Member

droidmonkey replied Nov 26, 2018

This commit was part of a much larger PR. #2351

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.