Extend krununittest to test ktelnetservice.
Details
- Reviewers
dfaure sitter meven feverfew ngraham - Group Reviewers
Frameworks - Commits
- R241:14b7f2c7ee72: [DesktopExecParser] Open {ssh,telnet,rlogin}:// urls with ktelnetservice
$ kde-open5 ssh://user@1.1.1.1
outputs: command= "ktelnetservice5 %u" args= ("ktelnetservice5 %u", "ssh://user@1.1.1.1") kf5.kio.core: couldn't create slave: "klauncher said: Unknown protocol 'ssh'.\n"
and you get a message box with "File not found: ssh://user@1.1.1.1".
Apply diff then try again, now a terminal window is launched by
ktelnetservice5 and ssh is invoked as expected.
BUG: 418258
FIXED-IN: 5.69
Diff Detail
- Repository
- R241 KIO
- Branch
- l-krun-ssh (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 23633 Build 23651: arc lint + arc unit
Tested fuse and it works as expected. Code looks reasonable, but I have little expertise with this class.
@feverfew probably ought to comment as well since he introduced the fuse code.
Thanks for the unittest with the fix.
autotests/krununittest.cpp | ||
---|---|---|
238 | QFINDTESTDATA() is more flexible than a solution like CMAKE_SOURCE_DIR [e.g. it allows deploying unittests to android devices]. It's also simpler to use (no buildsystem change). |
autotests/krununittest.cpp | ||
---|---|---|
238 | Good practice is to follow that with QVERIFY(!ktelnet.isEmpty()); | |
245 | Wait, I didn't notice before, but this is broken. |
- Use QStandardPaths::findExecutable(), as the path isn't hardcoded to /usr/bin/ on all systems
- More QVERIFY()
Thanks, looks better. One possible problem left: if KIO has never been installed, I guess QStandardPaths::findExecutable(QStringLiteral("ktelnetservice5")) won't find it in builddir/bin.
You could push and see what CI says, or you could test locally what happens when running this test method with PATH=/does/not/exist.
It seems to me that desktopexecparser.cpp won't find it then, which should probably be fixed with a fallback to "if it exists in QCoreApplication::applicationDirPath(), use that" -- and then the unittest can do the same.
As pointed out by dfaure, we have to make sure the unit test passes even if KIO isn't installed (which might be the case on the KDE CI system). Since we aren't actually executing the ktelnetservice5 binary and only checking what DesktopExecParser will try to use, this should work.
don't merge yet, looking at the code I sense something might break here but I need some time to do my own testing first.
OK with me, but check with the fuse people.
You test ssh, telnet etc but hasSchemeHandler is also true for at least http.
While I was debugging this issue, I found that the code gets here starting from this runApplication() call https://cgit.kde.org/kio.git/tree/src/widgets/krun.cpp#n843, which, IIUC, means that "http" is caught in KRun::init() before we ever get here.
Right, I missed the bit about the external browser; however with the recent change to fallback to mimeapps.list if external browser is empty, it "should" be OK...
Quick testing with fish protocol doesn't break anything KIOFuse side. Going to page in @ngraham, if it works for him with smb it's an all good from the "KIOFuse people" ;)
Sorry for the delay!