D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
hallas closed this revision. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D23884 To: hallas, dfaure Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R244 KCoreAddons BRANCH fix_unittest_if_lsof_is_not_installed REVISION DETAIL https://phabricator.kde.org/D23884 To: hallas, dfaure Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
hallas marked 2 inline comments as done. hallas added inline comments. INLINE COMMENTS > dfaure wrote in klistopenfilesjobtest_unix.cpp:67 > Please use QSKIP instead, otherwise the test output looks like the test > passed, rather than was skipped. Good point, I didn't know that :) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D23884 To: hallas, dfaure Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
hallas updated this revision to Diff 65937. hallas added a comment. Use QSKIP to skip tests REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23884?vs=65870=65937 BRANCH fix_unittest_if_lsof_is_not_installed REVISION DETAIL https://phabricator.kde.org/D23884 AFFECTED FILES autotests/klistopenfilesjobtest_unix.cpp To: hallas, dfaure Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Meanwhile lsof was installed on the Linux CI nodes, but this fix is still
good to have of course, just in case.
INLINE COMMENTS
> klistopenfilesjobtest_unix.cpp:34
> +
> +bool HasLsofInstalled()
> +{
Method names start with a lowercase letter in Qt/KDE code.
> klistopenfilesjobtest_unix.cpp:67
> +if (!HasLsofInstalled()) {
> +qDebug() << "lsof is not installed - skipping test";
> +return;
Please use QSKIP instead, otherwise the test output looks like the test passed,
rather than was skipped.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D23884
To: hallas, dfaure
Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
hallas updated this revision to Diff 65870. hallas added a comment. Use QStandardPaths::findExecutable to locate lsof REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23884?vs=65869=65870 BRANCH fix_unittest_if_lsof_is_not_installed REVISION DETAIL https://phabricator.kde.org/D23884 AFFECTED FILES autotests/klistopenfilesjobtest_unix.cpp To: hallas, dfaure Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
hallas added inline comments. INLINE COMMENTS > dhaumann wrote in klistopenfilesjobtest_unix.cpp:34 > Can't you simply use QStandardPaths::findExecutable()? > > https://doc.qt.io/qt-5/qstandardpaths.html#findExecutable Yes, this seems to do exactly what we need :) > ngraham wrote in klistopenfilesjobtest_unix.cpp:37 > A prior career in build engineering tells me that this will start failing in > 10 years when `lsof` removes the `-v` argument and replaces it with > `--version`. For robustness' sake, I would additionally check for `--version` > (and possibly even `-version` too) if the initial `lsof -v` call fails. I have changed the code to use QStandardPaths::findExecutable instead REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D23884 To: hallas, dfaure Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
dhaumann added inline comments.
INLINE COMMENTS
> klistopenfilesjobtest_unix.cpp:34
> +
> +bool HasLsofInstalled()
> +{
Can't you simply use QStandardPaths::findExecutable()?
https://doc.qt.io/qt-5/qstandardpaths.html#findExecutable
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D23884
To: hallas, dfaure
Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
ngraham added inline comments.
INLINE COMMENTS
> klistopenfilesjobtest_unix.cpp:37
> +QProcess lsofProcess;
> +lsofProcess.start(QStringLiteral("lsof"),
> QStringList(QStringLiteral("-v")));
> +lsofProcess.waitForFinished();
A prior career in build engineering tells me that this will start failing in 10
years when `lsof` removes the `-v` argument and replaces it with `--version`.
For robustness' sake, I would additionally check for `--version` (and possibly
even `-version` too) if the initial `lsof -v` call fails.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D23884
To: hallas, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed
hallas created this revision. hallas added a reviewer: dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY The KListOpenFilesJob unit test on Unix expected lsof to be installed, but the KDE build infrastructure doesn't have it installed, so check if it is installed and skip the tests if it is not. TEST PLAN Unit Test REPOSITORY R244 KCoreAddons BRANCH fix_unittest_if_lsof_is_not_installed REVISION DETAIL https://phabricator.kde.org/D23884 AFFECTED FILES autotests/klistopenfilesjobtest_unix.cpp To: hallas, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
