Add some tests for FolderModel and Positioner classes.
Details
Details
- Reviewers
ervin hein mlaurent aacid dvratil franckarrecot renatoo olivierjg - Group Reviewers
Plasma - Commits
- R119:c681a2f6ef63: Add unit tests for Folder View
Diff Detail
Diff Detail
- Repository
- R119 Plasma Desktop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Add in toplevel:
if(BUILD_TESTING)
add_definitions(-DBUILD_TESTING)
endif(BUILD_TESTING)
+
create a folderplugin_private_export.h as
#ifndef KSIEVEUIPRIVATE_EXPORT_H
#define KSIEVEUIPRIVATE_EXPORT_H
#include "ksieveui_export.h"
/* Classes which are exported only for unit tests */
#ifdef BUILD_TESTING
- ifndef FOLDERPLUGIN_TESTS_EXPORT
- define FOLDERPLUGIN_TESTS_EXPORT FOLDERPLUGIN_EXPORT
- endif
#else /* not compiling tests */
define FOLDERPLUGIN_TESTS_EXPORT
#endif
#endif
> we avoid to export symbol when we don't build test => distro doesn't generate lib with unnecessary exported symbol.
| containments/desktop/plugins/folder/CMakeLists.txt | ||
|---|---|---|
| 39 | Use add_subdirectory(tests) endif() it's avoid to build tests when distro builds it. | |
| containments/desktop/plugins/folder/folderplugin_private_export.h | ||
|---|---|---|
| 2 | Missing copyright | |
| containments/desktop/plugins/folder/tests/foldermodeltest.cpp | ||
| 2 | Missing copyright | |
| 88 | QStringLiteral(...).arg | |
| 103 | QStringLiteral(...).arg(...) | |
| 133 | QStringLiteral("...) same for other line | |
| containments/desktop/plugins/folder/tests/foldermodeltest.h | ||
| 2 | missing copyright | |
| containments/desktop/plugins/folder/tests/positionertest.cpp | ||
| 2 | copyright | |
| 54 | nullptr; | |
| 56 | nullptr; | |
| containments/desktop/plugins/folder/tests/positionertest.h | ||
| 2 | missing copyright | |
Comment Actions
Thanks for working on this! We really need better test coverage in Plasma bits.
I'm a little bit unhappy about littering headers with FOLDERPLUGIN_TESTS_EXPORT, but I guess it's the lesser evil of statically linking and building the code twice.
The tests itself look like a good start.
(Aside: If you have bigger plans for upcoming FV contributions, it'd be cool to sit down on IRC and talk a bit. I had a long conversation with Milian last month, I guess/hope he passed that info on.)
Comment Actions
Please don't extend already-accepted review requests with new code without requesting more review. Otherwise the status stays "Accepted" on code that hasn't actually been reviewed, which is confusing and holds things up :)
New test looks fine, though.
Comment Actions
I approved and actually suggested Laurent to update the diff. Sorry for the confusion.
Comment Actions
It looks like it broke building with BUILD_TESTING=OFF:
/home/abuild/rpmbuild/BUILD/plasma-desktop-5.11.90git.20171028T043842~44827507/containments/desktop/plugins/folder/foldermodel.h:74:45: error: expected initializer before ':' token