Extend krununittest to test ktelnetservice.
Details
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
Diff Detail
- Repository
- R241 KIO
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
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.
Comment Actions
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. | |
Comment Actions
- Use QStandardPaths::findExecutable(), as the path isn't hardcoded to /usr/bin/ on all systems
- More QVERIFY()
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
OK with me, but check with the fuse people.
You test ssh, telnet etc but hasSchemeHandler is also true for at least http.
Comment Actions
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.
Comment Actions
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...
Comment Actions
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!
This comment was removed by ahmadsamir.