Fix KListOpenFilesJob unit test on Unix if lsof is not installed
ClosedPublic

Authored by hallas on Sep 11 2019, 5:35 PM.

Details

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

Diff Detail

Repository
R244 KCoreAddons
Branch
fix_unittest_if_lsof_is_not_installed
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16395
Build 16413: arc lint + arc unit
hallas created this revision.Sep 11 2019, 5:35 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 11 2019, 5:35 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
hallas requested review of this revision.Sep 11 2019, 5:35 PM
ngraham added inline comments.
autotests/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.

dhaumann added inline comments.
autotests/klistopenfilesjobtest_unix.cpp
34

Can't you simply use QStandardPaths::findExecutable()?

https://doc.qt.io/qt-5/qstandardpaths.html#findExecutable

hallas updated this revision to Diff 65870.Sep 11 2019, 5:57 PM

Use QStandardPaths::findExecutable to locate lsof

hallas added inline comments.Sep 11 2019, 5:57 PM
autotests/klistopenfilesjobtest_unix.cpp
34

Yes, this seems to do exactly what we need :)

37

I have changed the code to use QStandardPaths::findExecutable instead

dfaure requested changes to this revision.Sep 12 2019, 9:57 AM

Meanwhile lsof was installed on the Linux CI nodes, but this fix is still good to have of course, just in case.

autotests/klistopenfilesjobtest_unix.cpp
34

Method names start with a lowercase letter in Qt/KDE code.

70

Please use QSKIP instead, otherwise the test output looks like the test passed, rather than was skipped.

This revision now requires changes to proceed.Sep 12 2019, 9:57 AM
hallas updated this revision to Diff 65937.Sep 12 2019, 5:32 PM

Use QSKIP to skip tests

hallas marked 2 inline comments as done.Sep 12 2019, 5:32 PM
hallas added inline comments.
autotests/klistopenfilesjobtest_unix.cpp
70

Good point, I didn't know that :)

dfaure accepted this revision.Sat, Sep 14, 12:17 PM
This revision is now accepted and ready to land.Sat, Sep 14, 12:17 PM
hallas closed this revision.Tue, Sep 17, 4:19 AM