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

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

Details

Reviewers
dfaure
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 16443
Build 16461: arc lint + arc unit
hallas created this revision.Wed, Sep 11, 5:35 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptWed, Sep 11, 5:35 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
hallas requested review of this revision.Wed, Sep 11, 5:35 PM
ngraham added inline comments.
autotests/klistopenfilesjobtest_unix.cpp
36

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
33

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

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

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

Use QStandardPaths::findExecutable to locate lsof

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

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

36

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

dfaure requested changes to this revision.Thu, Sep 12, 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
33

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

65

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

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

Use QSKIP to skip tests

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

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