Changeset View
Standalone View
autotests/jobtest.cpp
Show All 27 Lines | |||||
28 | #include <QEventLoop> | 28 | #include <QEventLoop> | ||
29 | #include <QDir> | 29 | #include <QDir> | ||
30 | #include <QHash> | 30 | #include <QHash> | ||
31 | #include <QVariant> | 31 | #include <QVariant> | ||
32 | #include <QBuffer> | 32 | #include <QBuffer> | ||
33 | #include <QTemporaryFile> | 33 | #include <QTemporaryFile> | ||
34 | #include <QTest> | 34 | #include <QTest> | ||
35 | #include <QUrl> | 35 | #include <QUrl> | ||
36 | #include <QProcess> | ||||
36 | 37 | | |||
37 | #include <kprotocolinfo.h> | 38 | #include <kprotocolinfo.h> | ||
38 | #include <kio/scheduler.h> | 39 | #include <kio/scheduler.h> | ||
39 | #include <kio/directorysizejob.h> | 40 | #include <kio/directorysizejob.h> | ||
40 | #include <kio/copyjob.h> | 41 | #include <kio/copyjob.h> | ||
41 | #include <kio/deletejob.h> | 42 | #include <kio/deletejob.h> | ||
42 | #include <kio/chmodjob.h> | 43 | #include <kio/chmodjob.h> | ||
43 | #include "kiotesthelper.h" // createTestFile etc. | 44 | #include "kiotesthelper.h" // createTestFile etc. | ||
▲ Show 20 Lines • Show All 52 Lines • ▼ Show 20 Line(s) | 80 | { | |||
96 | } | 97 | } | ||
97 | #if 0 | 98 | #if 0 | ||
98 | if (KProtocolInfo::isKnownProtocol("system")) { | 99 | if (KProtocolInfo::isKnownProtocol("system")) { | ||
99 | if (!QFile::exists(realSystemPath())) { | 100 | if (!QFile::exists(realSystemPath())) { | ||
100 | bool ok = dir.mkdir(realSystemPath()); | 101 | bool ok = dir.mkdir(realSystemPath()); | ||
101 | if (!ok) { | 102 | if (!ok) { | ||
102 | qFatal("couldn't create %s", qPrintable(realSystemPath())); | 103 | qFatal("couldn't create %s", qPrintable(realSystemPath())); | ||
103 | } | 104 | } | ||
104 | } | 105 | } | ||
bruns: Whats the reason to split off the path?
Also, you can not just concatenate the command and args… | |||||
105 | } | 106 | } | ||
106 | #endif | 107 | #endif | ||
107 | 108 | | |||
108 | qRegisterMetaType<KJob *>("KJob*"); | 109 | qRegisterMetaType<KJob *>("KJob*"); | ||
109 | qRegisterMetaType<KIO::Job *>("KIO::Job*"); | 110 | qRegisterMetaType<KIO::Job *>("KIO::Job*"); | ||
110 | qRegisterMetaType<QDateTime>("QDateTime"); | 111 | qRegisterMetaType<QDateTime>("QDateTime"); | ||
111 | } | 112 | } | ||
112 | 113 | | |||
113 | void JobTest::cleanupTestCase() | 114 | void JobTest::cleanupTestCase() | ||
114 | { | 115 | { | ||
115 | QDir(homeTmpDir()).removeRecursively(); | 116 | QDir(homeTmpDir()).removeRecursively(); | ||
116 | QDir(otherTmpDir()).removeRecursively(); | 117 | QDir(otherTmpDir()).removeRecursively(); | ||
117 | #if 0 | 118 | #if 0 | ||
typo in Xatr, and in "foud". I suggest: qWarning() << "Neither getfattr, getextattr nor xattr was found"; Although I have to wonder why this looks for all three, to then only use getfattr and skip tests in all other cases.... dfaure: typo in Xatr, and in "foud". I suggest:
qWarning() << "Neither getfattr, getextattr nor… | |||||
The check is to build infrastructure for anyone with access to the platforms that want to add their test. cochise: The check is to build infrastructure for anyone with access to the platforms that want to add… | |||||
OK. But then please use my qWarning suggestion, it matches reality better, and will be less confusing for people testing this on other platforms. dfaure: OK. But then please use my qWarning suggestion, it matches reality better, and will be less… | |||||
118 | if (KProtocolInfo::isKnownProtocol("system")) { | 119 | if (KProtocolInfo::isKnownProtocol("system")) { | ||
119 | delDir(systemTmpDir()); | 120 | delDir(systemTmpDir()); | ||
120 | } | 121 | } | ||
121 | #endif | 122 | #endif | ||
122 | } | 123 | } | ||
123 | 124 | | |||
124 | void JobTest::enterLoop() | 125 | void JobTest::enterLoop() | ||
125 | { | 126 | { | ||
▲ Show 20 Lines • Show All 319 Lines • ▼ Show 20 Line(s) | |||||
445 | //// | 446 | //// | ||
446 | 447 | | |||
447 | void JobTest::copyLocalFile(const QString &src, const QString &dest) | 448 | void JobTest::copyLocalFile(const QString &src, const QString &dest) | ||
448 | { | 449 | { | ||
449 | const QUrl u = QUrl::fromLocalFile(src); | 450 | const QUrl u = QUrl::fromLocalFile(src); | ||
450 | const QUrl d = QUrl::fromLocalFile(dest); | 451 | const QUrl d = QUrl::fromLocalFile(dest); | ||
451 | 452 | | |||
452 | const int perms = 0666; | 453 | const int perms = 0666; | ||
453 | // copy the file with file_copy | 454 | // copy the file with file_copy | ||
pino: extra empty line | |||||
this obviously needs test cases with a key ley/value len > 512, to check the the array-to-short/resize path. bruns: this obviously needs test cases with a key ley/value len > 512, to check the the array-to… | |||||
bruns: not done ... | |||||
arrowd: There are no arrays of `512` size anymore. Or am I missing something? | |||||
454 | KIO::Job *job = KIO::file_copy(u, d, perms, KIO::HideProgressInfo); | 455 | KIO::Job *job = KIO::file_copy(u, d, perms, KIO::HideProgressInfo); | ||
455 | job->setUiDelegate(nullptr); | 456 | job->setUiDelegate(nullptr); | ||
dfaure: this method has weird indentation | |||||
456 | bool ok = job->exec(); | 457 | bool ok = job->exec(); | ||
457 | QVERIFY(ok); | 458 | QVERIFY(ok); | ||
458 | QVERIFY(QFile::exists(dest)); | 459 | QVERIFY(QFile::exists(dest)); | ||
459 | QVERIFY(QFile::exists(src)); // still there | 460 | QVERIFY(QFile::exists(src)); // still there | ||
usta: const ? | |||||
460 | QCOMPARE(int(QFileInfo(dest).permissions()), int(0x6666)); | 461 | QCOMPARE(int(QFileInfo(dest).permissions()), int(0x6666)); | ||
461 | 462 | | |||
const QString And if you just call this with the target directory (e.g. homeTmpDir()), you can skip the split/join dance. bruns: const QString
And if you just call this with the target directory (e.g. ` homeTmpDir()`), you… | |||||
462 | { | 463 | { | ||
463 | // check that the timestamp is the same (#24443) | 464 | // check that the timestamp is the same (#24443) | ||
464 | // Note: this only works because of copy() in kio_file. | 465 | // Note: this only works because of copy() in kio_file. | ||
465 | // The datapump solution ignores mtime, the app has to call FileCopyJob::setModificationTime() | 466 | // The datapump solution ignores mtime, the app has to call FileCopyJob::setModificationTime() | ||
bruns: should be `dest`. | |||||
466 | QFileInfo srcInfo(src); | 467 | QFileInfo srcInfo(src); | ||
467 | QFileInfo destInfo(dest); | 468 | QFileInfo destInfo(dest); | ||
468 | #ifdef Q_OS_WIN | 469 | #ifdef Q_OS_WIN | ||
469 | // win32 time may differs in msec part | 470 | // win32 time may differs in msec part | ||
470 | QCOMPARE(srcInfo.lastModified().toString("dd.MM.yyyy hh:mm"), | 471 | QCOMPARE(srcInfo.lastModified().toString("dd.MM.yyyy hh:mm"), | ||
471 | destInfo.lastModified().toString("dd.MM.yyyy hh:mm")); | 472 | destInfo.lastModified().toString("dd.MM.yyyy hh:mm")); | ||
472 | #else | 473 | #else | ||
473 | QCOMPARE(srcInfo.lastModified(), destInfo.lastModified()); | 474 | QCOMPARE(srcInfo.lastModified(), destInfo.lastModified()); | ||
474 | #endif | 475 | #endif | ||
475 | } | 476 | } | ||
aren't we using camelcase rules for variables in kde ? is so xattrwriter => xattrWriter usta: aren't we using camelcase rules for variables in kde ? is so xattrwriter => xattrWriter | |||||
476 | 477 | | |||
477 | // cleanup and retry with KIO::copy() | 478 | // cleanup and retry with KIO::copy() | ||
std::function<QStringList(const QString&, const QString&, const QString&)> m_setXattrFormatArgs; m_setXattrFormatArgs = [](const QString& attrName, const QString& value, const QString& fileName) { return QStringList{QLatin1String("-n"), attrName, QLatin1String("-v"), value, fileName}; } and do it when doing the path lookup, once. bruns: `std::function<QStringList(const QString&, const QString&, const QString&)>… | |||||
478 | QFile::remove(dest); | 479 | QFile::remove(dest); | ||
479 | job = KIO::copy(u, d, KIO::HideProgressInfo); | 480 | job = KIO::copy(u, d, KIO::HideProgressInfo); | ||
480 | QSignalSpy spyCopyingDone(job, SIGNAL(copyingDone(KIO::Job*,QUrl,QUrl,QDateTime,bool,bool))); | 481 | QSignalSpy spyCopyingDone(job, SIGNAL(copyingDone(KIO::Job*,QUrl,QUrl,QDateTime,bool,bool))); | ||
481 | job->setUiDelegate(nullptr); | 482 | job->setUiDelegate(nullptr); | ||
482 | job->setUiDelegateExtension(nullptr); | 483 | job->setUiDelegateExtension(nullptr); | ||
483 | ok = job->exec(); | 484 | ok = job->exec(); | ||
bruns: Should have a `break` or `return` here. | |||||
484 | QVERIFY(ok); | 485 | QVERIFY(ok); | ||
485 | QVERIFY(QFile::exists(dest)); | 486 | QVERIFY(QFile::exists(dest)); | ||
486 | QVERIFY(QFile::exists(src)); // still there | 487 | QVERIFY(QFile::exists(src)); // still there | ||
487 | { | 488 | { | ||
bruns: This seem bogus to me - what guarantees the process has not already finished? | |||||
488 | // check that the timestamp is the same (#24443) | 489 | // check that the timestamp is the same (#24443) | ||
489 | QFileInfo srcInfo(src); | 490 | QFileInfo srcInfo(src); | ||
490 | QFileInfo destInfo(dest); | 491 | QFileInfo destInfo(dest); | ||
491 | #ifdef Q_OS_WIN | 492 | #ifdef Q_OS_WIN | ||
492 | // win32 time may differs in msec part | 493 | // win32 time may differs in msec part | ||
493 | QCOMPARE(srcInfo.lastModified().toString("dd.MM.yyyy hh:mm"), | 494 | QCOMPARE(srcInfo.lastModified().toString("dd.MM.yyyy hh:mm"), | ||
494 | destInfo.lastModified().toString("dd.MM.yyyy hh:mm")); | 495 | destInfo.lastModified().toString("dd.MM.yyyy hh:mm")); | ||
495 | #else | 496 | #else | ||
496 | QCOMPARE(srcInfo.lastModified(), destInfo.lastModified()); | 497 | QCOMPARE(srcInfo.lastModified(), destInfo.lastModified()); | ||
497 | #endif | 498 | #endif | ||
498 | } | 499 | } | ||
499 | QCOMPARE(spyCopyingDone.count(), 1); | 500 | QCOMPARE(spyCopyingDone.count(), 1); | ||
void JobTest::compareXattr(const QString &src, const QString &dest) { auto srcAttrs = readXattr(src); auto destAttrs = readXattr(dest); QCOMPARE(srcAttrs, destAttrs); } QList<QByteArray> JobTest::readXattr(const QString &path) { bruns: ```
void JobTest::compareXattr(const QString &src, const QString &dest)
{
auto srcAttrs =… | |||||
500 | 501 | | |||
501 | // cleanup and retry with KIO::copyAs() | 502 | // cleanup and retry with KIO::copyAs() | ||
502 | QFile::remove(dest); | 503 | QFile::remove(dest); | ||
503 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | 504 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | ||
504 | job->setUiDelegate(nullptr); | 505 | job->setUiDelegate(nullptr); | ||
505 | job->setUiDelegateExtension(nullptr); | 506 | job->setUiDelegateExtension(nullptr); | ||
506 | QVERIFY(job->exec()); | 507 | QVERIFY(job->exec()); | ||
usta: camelCase ? xattrreader => xattrReader | |||||
usta: typo :) xattRreader => xattrReader | |||||
cochise: I really need to sleep, rs. | |||||
507 | QVERIFY(QFile::exists(dest)); | 508 | QVERIFY(QFile::exists(dest)); | ||
508 | QVERIFY(QFile::exists(src)); // still there | 509 | QVERIFY(QFile::exists(src)); // still there | ||
509 | 510 | | |||
510 | // Do it again, with Overwrite. | 511 | // Do it again, with Overwrite. | ||
511 | job = KIO::copyAs(u, d, KIO::Overwrite | KIO::HideProgressInfo); | 512 | job = KIO::copyAs(u, d, KIO::Overwrite | KIO::HideProgressInfo); | ||
bruns: dito | |||||
512 | job->setUiDelegate(nullptr); | 513 | job->setUiDelegate(nullptr); | ||
513 | job->setUiDelegateExtension(nullptr); | 514 | job->setUiDelegateExtension(nullptr); | ||
514 | QVERIFY(job->exec()); | 515 | QVERIFY(job->exec()); | ||
515 | QVERIFY(QFile::exists(dest)); | 516 | QVERIFY(QFile::exists(dest)); | ||
516 | QVERIFY(QFile::exists(src)); // still there | 517 | QVERIFY(QFile::exists(src)); // still there | ||
517 | 518 | | |||
518 | // Do it again, without Overwrite (should fail). | 519 | // Do it again, without Overwrite (should fail). | ||
519 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | 520 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | ||
520 | job->setUiDelegate(nullptr); | 521 | job->setUiDelegate(nullptr); | ||
521 | job->setUiDelegateExtension(nullptr); | 522 | job->setUiDelegateExtension(nullptr); | ||
522 | QVERIFY(!job->exec()); | 523 | QVERIFY(!job->exec()); | ||
523 | 524 | | |||
524 | // Clean up | 525 | // Clean up | ||
525 | QFile::remove(dest); | 526 | QFile::remove(dest); | ||
526 | } | 527 | } | ||
527 | 528 | | |||
529 | QHash<QString, QString> getSampleAttrs() | ||||
dfaure: prepend `static` | |||||
530 | { | ||||
531 | QHash<QString, QString> attrs; | ||||
532 | attrs["user.name with space"] = "value with spaces"; | ||||
533 | attrs["user.baloo.rating"] = "1"; | ||||
you can use QStandardPaths::findExecutable() to check whether setfattr is available, and QSKIP the test if not pino: you can use `QStandardPaths::findExecutable()` to check whether `setfattr` is available, and… | |||||
On linux, the command line bin are list/get/setfattr, on BSD list/get/setxattr, on mac are xattr -l/-p/-w, so this part will need a lot of ifdefs too, to work on many platforms. cochise: On linux, the command line bin are list/get/set**f**attr, on BSD list/get/set**x**attr, on mac… | |||||
534 | attrs["user.fnewLine"] = "line1\\nline2"; | ||||
535 | attrs["user.flistNull"] = "item1\\0item2"; | ||||
dfaure: QVERIFY(....) around all calls to waitForFinished
(repeats) | |||||
536 | attrs["user.fattr.with.a.lot.of.namespaces"] = "true"; | ||||
537 | attrs["root.fnot.allowed"] = "forbidden"; | ||||
This entire QHash is duplicated as is between both methods, right? Extract it into a function that returns the QHash (by value). dfaure: This entire QHash is duplicated as is between both methods, right?
Extract it into a function… | |||||
538 | attrs["user.name with space"] = "value with spaces"; | ||||
539 | return attrs; | ||||
please use the QProcess::start(QString,QStringList) variant, splitting the arguments; this way there is no need to manual quoting the paths, and thus avoid problems with spaces pino: please use the `QProcess::start(QString,QStringList)` variant, splitting the arguments; this… | |||||
pino: check using `QVERIFY`/`QCOMPARE` that the process was started correctly | |||||
540 | } | ||||
pino: check using `QVERIFY`/`QCOMPARE` that the process ended correctly | |||||
541 | | ||||
542 | void JobTest::setXattr(const QString &src, const QString &command) | ||||
543 | { | ||||
544 | QProcess xattrwriter; | ||||
545 | | ||||
546 | QStringList arguments = {"-n", "user.fnoValue", src}; | ||||
547 | xattrwriter.start(command, arguments); | ||||
548 | QVERIFY(xattrwriter.waitForFinished(-1)); | ||||
549 | | ||||
550 | QHash<QString, QString> attrs = getSampleAttrs(); | ||||
551 | | ||||
552 | arguments.insert(2, "-v"); | ||||
553 | arguments.insert(3, "value"); | ||||
554 | // arguments 0:"-n" 1:"name" 2:"-v", 3:"value" 4:src | ||||
555 | QHashIterator<QString, QString> i(attrs); | ||||
556 | while (i.hasNext()) { | ||||
Whenever you see QVERIFY(a==b) it should in fact be QCOMPARE(a, b) so that we can see the value on failure (this isn't gtest). If needed, cast to int. [repeats] dfaure: Whenever you see QVERIFY(a==b) it should in fact be QCOMPARE(a, b) so that we can see the value… | |||||
557 | i.next(); | ||||
558 | arguments.replace(1, i.key()); | ||||
559 | arguments.replace(3, i.value()); | ||||
560 | xattrwriter.start(command, arguments); | ||||
561 | QVERIFY(xattrwriter.waitForStarted()); | ||||
562 | QCOMPARE(xattrwriter.state(), QProcess::Running); | ||||
563 | QVERIFY(xattrwriter.waitForFinished(-1)); | ||||
564 | QCOMPARE(xattrwriter.exitStatus(), QProcess::NormalExit); | ||||
565 | } | ||||
566 | } | ||||
567 | | ||||
568 | void JobTest::compareXattr(const QString &src, const QString &dest, QString &command) | ||||
remove the command parameter from the method, use e.g. a m_xattrGetCommand JobTest member bruns: remove the command parameter from the method, use e.g. a `m_xattrGetCommand` JobTest member | |||||
569 | { | ||||
570 | if (command == "setfattr"){ | ||||
571 | command = "getfattr"; | ||||
572 | } else if (command == "setextattr") { | ||||
573 | command = "getextatr"; | ||||
I am not sure but it looks like there is a typo here usta: I am not sure but it looks like there is a typo here
getextatr --->
getextattr
but not sure… | |||||
{set,get}fattr = linux command This command could have a little more consistency... cochise: {set,get}fattr = linux command
{set.get}extattr = BSD command
{set.get}xtattr = MacOS command… | |||||
cochise: xtattr = MacOS command
It uses a single command to read and write.
| |||||
All of these are spelled with two consecutive 't'. dfaure: All of these are spelled with two consecutive 't'.
The code has a single 't' : getextatr. | |||||
usta: but you have written it as with single T , i think it must be double ( TT ) | |||||
cochise: You are right. Sorry.
Fixed. =] | |||||
574 | } | ||||
575 | | ||||
576 | QProcess xattrreader; | ||||
577 | xattrreader.setProcessChannelMode(QProcess::MergedChannels); | ||||
578 | | ||||
579 | //test destination to see if filesystem supports xattr | ||||
580 | xattrreader.start(command, QStringList{"-n", "user.test", dest}); | ||||
581 | QVERIFY(xattrreader.waitForFinished(-1)); | ||||
582 | QString testfs = xattrreader.readAllStandardOutput(); | ||||
583 | if (testfs.section(':', 2, 2) == " Operation not supported\n") { | ||||
584 | return; | ||||
585 | } else { | ||||
586 | //clean up | ||||
587 | xattrreader.start(command, QStringList{"-x", "user.test", dest}); | ||||
588 | QVERIFY(xattrreader.waitForFinished(-1)); | ||||
589 | } | ||||
590 | | ||||
591 | QHash<QString, QString> attrs = getSampleAttrs(); | ||||
592 | | ||||
593 | QStringList arguments = {"-n", "name", src}; | ||||
By just iterating over the expected xattrs, extraneous attrs will be silently skipped. Use '-d' bruns: By just iterating over the expected xattrs, extraneous attrs will be silently skipped. Use '-d' | |||||
594 | QHashIterator<QString, QString> i(attrs); | ||||
595 | while (i.hasNext()) { | ||||
596 | i.next(); | ||||
597 | arguments.replace(1, i.key()); | ||||
598 | xattrreader.start(command, arguments); | ||||
599 | QVERIFY(xattrreader.waitForStarted()); | ||||
600 | QCOMPARE(xattrreader.state(), QProcess::Running); | ||||
601 | QVERIFY(xattrreader.waitForFinished(-1)); | ||||
602 | QCOMPARE(xattrreader.exitStatus(), QProcess::NormalExit); | ||||
603 | QString resultsrc = xattrreader.readAllStandardOutput(); | ||||
604 | /****** | ||||
605 | * We need to chop the filename from command output to compare | ||||
606 | * 0:"getfattr: Removing leading '/' from absolute path names\n | ||||
607 | * 1:# file: home/user/.qttest/share/kio/jobtest/kiotests/fileFromHome\n | ||||
608 | * 2:user.baloo.rating=\"1\"\n | ||||
609 | * 3:\n" | ||||
610 | * ****/ | ||||
611 | resultsrc = resultsrc.section('\n', 2, 2); | ||||
612 | | ||||
613 | arguments.replace(2, dest); | ||||
Redundant code. Call this function once after writing the source, compare with what you tried to write. Call another time for each copy operation. bruns: Redundant code.
Call this function once after writing the source, compare with what you tried… | |||||
614 | xattrreader.start(command, arguments); | ||||
615 | QVERIFY(xattrreader.waitForStarted()); | ||||
616 | QCOMPARE(xattrreader.state(), QProcess::Running); | ||||
617 | QVERIFY(xattrreader.waitForFinished(-1)); | ||||
618 | QCOMPARE(xattrreader.exitStatus(), QProcess::NormalExit); | ||||
619 | QString resultdest = xattrreader.readAllStandardOutput(); | ||||
620 | resultdest = resultdest.section('\n', 2, 2); | ||||
621 | | ||||
622 | QCOMPARE(resultdest, resultsrc); | ||||
623 | } | ||||
624 | } | ||||
625 | | ||||
626 | QString getXattrCommand() | ||||
dfaure: prepend `static` | |||||
bruns: Do the lookup once in `JobTest::initTestCase` | |||||
bruns: Check for both set and get commands. | |||||
627 | { | ||||
628 | /***** | ||||
629 | * Find if the platform have support for xattr. | ||||
630 | * Linux commands: setfattr, getfattr | ||||
631 | * BSD commands: setextattr, getextattr | ||||
632 | * MacOS commands: xattr -w, xattr -p | ||||
633 | ****/ | ||||
634 | QString command = QStandardPaths::findExecutable("setfattr"); | ||||
635 | if (command.isEmpty()) { | ||||
636 | command = QStandardPaths::findExecutable("setextattr"); | ||||
637 | if (command.isEmpty()) { | ||||
638 | command = QStandardPaths::findExecutable("xattr"); | ||||
if (command.isEmpty()) qWarning() << "ERROR: setfattr/setextattr/xattr not found"; to make it easier to debug than just this method returning empty. dfaure: if (command.isEmpty())
qWarning() << "ERROR: setfattr/setextattr/xattr not found"… | |||||
dfaure: Was my comment ignored? | |||||
cochise: Fixed on new commit. | |||||
639 | } | ||||
640 | } | ||||
641 | return command.split("/").last(); | ||||
bruns: Keep the full path. | |||||
642 | } | ||||
643 | | ||||
644 | void JobTest::copyLocalFileWithXattr(const QString &src, const QString &dest) | ||||
645 | { | ||||
646 | QString command = getXattrCommand(); | ||||
647 | | ||||
648 | // TODO: The tests are linux only for now. | ||||
649 | if (command.isEmpty() || command != "setfattr") { | ||||
650 | return; | ||||
QSKIP("This test requires setfattr") Otherwise it will look like a "PASS" in the test output. dfaure: QSKIP("This test requires setfattr")
Otherwise it will look like a "PASS" in the test output. | |||||
dfaure: My comment was ignored. | |||||
651 | } | ||||
652 | | ||||
653 | setXattr(src, command); | ||||
654 | | ||||
655 | // Simplified version of tests on copyLocalFile with a added compareXattr call | ||||
656 | const QUrl u = QUrl::fromLocalFile(src); | ||||
657 | const QUrl d = QUrl::fromLocalFile(dest); | ||||
658 | | ||||
659 | // copy the file with file_copy | ||||
660 | const int perms = 0666; | ||||
dfaure: always use QVERIFY2(job->exec(), qPrintable(job->errorString()));
(repeats) | |||||
661 | KIO::Job *job = KIO::file_copy(u, d, perms, KIO::HideProgressInfo); | ||||
662 | job->setUiDelegate(nullptr); | ||||
663 | QVERIFY2(job->exec(), qPrintable(job->errorString())); | ||||
664 | compareXattr(src, dest, command); // Our test | ||||
665 | | ||||
666 | // cleanup and retry with KIO::copy() | ||||
667 | QFile::remove(dest); | ||||
668 | job = KIO::copy(u, d, KIO::HideProgressInfo); | ||||
669 | job->setUiDelegate(nullptr); | ||||
670 | job->setUiDelegateExtension(nullptr); | ||||
671 | QVERIFY2(job->exec(), qPrintable(job->errorString())); | ||||
672 | compareXattr(src, dest, command); // Our test | ||||
673 | | ||||
674 | // cleanup and retry with KIO::copyAs() | ||||
675 | QFile::remove(dest); | ||||
676 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | ||||
677 | job->setUiDelegate(nullptr); | ||||
678 | job->setUiDelegateExtension(nullptr); | ||||
679 | QVERIFY2(job->exec(), qPrintable(job->errorString())); | ||||
680 | compareXattr(src, dest, command); // Our test | ||||
681 | | ||||
682 | // Do it again, with Overwrite. | ||||
683 | job = KIO::copyAs(u, d, KIO::Overwrite | KIO::HideProgressInfo); | ||||
684 | job->setUiDelegate(nullptr); | ||||
685 | job->setUiDelegateExtension(nullptr); | ||||
686 | QVERIFY2(job->exec(), qPrintable(job->errorString())); | ||||
687 | compareXattr(src, dest, command); // Our test | ||||
bruns: If this copy never happened, the check would still succeed. | |||||
688 | | ||||
689 | // Clean up | ||||
690 | QFile::remove(dest); | ||||
691 | } | ||||
692 | | ||||
528 | void JobTest::copyLocalDirectory(const QString &src, const QString &_dest, int flags) | 693 | void JobTest::copyLocalDirectory(const QString &src, const QString &_dest, int flags) | ||
529 | { | 694 | { | ||
530 | QVERIFY(QFileInfo(src).isDir()); | 695 | QVERIFY(QFileInfo(src).isDir()); | ||
531 | QVERIFY(QFileInfo(src + "/testfile").isFile()); | 696 | QVERIFY(QFileInfo(src + "/testfile").isFile()); | ||
532 | QUrl u = QUrl::fromLocalFile(src); | 697 | QUrl u = QUrl::fromLocalFile(src); | ||
533 | QString dest(_dest); | 698 | QString dest(_dest); | ||
534 | QUrl d = QUrl::fromLocalFile(dest); | 699 | QUrl d = QUrl::fromLocalFile(dest); | ||
535 | if (flags & AlreadyExists) { | 700 | if (flags & AlreadyExists) { | ||
Show All 38 Lines | 729 | #endif | |||
574 | // Do it again, without Overwrite (should fail). | 739 | // Do it again, without Overwrite (should fail). | ||
575 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | 740 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | ||
576 | job->setUiDelegate(nullptr); | 741 | job->setUiDelegate(nullptr); | ||
577 | job->setUiDelegateExtension(nullptr); | 742 | job->setUiDelegateExtension(nullptr); | ||
578 | ok = job->exec(); | 743 | ok = job->exec(); | ||
579 | QVERIFY(!ok); | 744 | QVERIFY(!ok); | ||
580 | } | 745 | } | ||
581 | 746 | | |||
747 | void JobTest::copyLocalDirectoryWithXattr(const QString &src, const QString &_dest, int flags) | ||||
748 | { | ||||
749 | QString command = getXattrCommand(); | ||||
750 | | ||||
751 | // TODO: The tests are linux only for now. | ||||
752 | if (command.isEmpty() || command != "setfattr") { | ||||
753 | return; | ||||
dfaure: QSKIP | |||||
754 | } | ||||
755 | | ||||
756 | const QUrl u = QUrl::fromLocalFile(src); | ||||
757 | QString dest(_dest); | ||||
758 | const QUrl d = QUrl::fromLocalFile(dest); | ||||
759 | | ||||
760 | setXattr(src, command); | ||||
761 | | ||||
762 | if (flags & AlreadyExists) { | ||||
763 | dest += '/' + u.fileName(); | ||||
764 | //qDebug() << "Expecting dest=" << dest; | ||||
765 | } else { | ||||
766 | // clean result of previous copyLocalDirectory call | ||||
767 | QDir(dest).removeRecursively(); | ||||
dfaure: Please extract a function from those 12 lines, which are duplicated. | |||||
768 | } | ||||
769 | | ||||
dfaure: const | |||||
770 | // Simplified version of tests on copyLocalDirectory with a added compareXattr call | ||||
771 | KIO::Job *job = KIO::copy(u, d, KIO::HideProgressInfo); | ||||
dfaure: const | |||||
772 | job->setUiDelegate(nullptr); | ||||
773 | job->setUiDelegateExtension(nullptr); | ||||
774 | QVERIFY2(job->exec(), qPrintable(job->errorString())); | ||||
775 | compareXattr(src, dest, command); // Our test | ||||
776 | | ||||
777 | // Do it again, with Overwrite. | ||||
778 | // Use copyAs, we don't want a subdir inside d. | ||||
779 | job = KIO::copyAs(u, d, KIO::HideProgressInfo | KIO::Overwrite); | ||||
780 | job->setUiDelegate(nullptr); | ||||
781 | job->setUiDelegateExtension(nullptr); | ||||
782 | QVERIFY2(job->exec(), qPrintable(job->errorString())); | ||||
783 | compareXattr(src, dest, command); // Our test | ||||
784 | } | ||||
785 | | ||||
582 | #ifndef Q_OS_WIN | 786 | #ifndef Q_OS_WIN | ||
583 | static QString linkTarget(const QString &path) | 787 | static QString linkTarget(const QString &path) | ||
584 | { | 788 | { | ||
585 | // Use readlink on Unix because symLinkTarget turns relative targets into absolute (#352927) | 789 | // Use readlink on Unix because symLinkTarget turns relative targets into absolute (#352927) | ||
586 | char linkTargetBuffer[4096]; | 790 | char linkTargetBuffer[4096]; | ||
587 | const int n = readlink(QFile::encodeName(path).constData(), linkTargetBuffer, sizeof(linkTargetBuffer) - 1); | 791 | const int n = readlink(QFile::encodeName(path).constData(), linkTargetBuffer, sizeof(linkTargetBuffer) - 1); | ||
588 | if (n != -1) { | 792 | if (n != -1) { | ||
589 | linkTargetBuffer[n] = 0; | 793 | linkTargetBuffer[n] = 0; | ||
Show All 22 Lines | |||||
612 | #endif | 816 | #endif | ||
613 | 817 | | |||
614 | void JobTest::copyFileToSamePartition() | 818 | void JobTest::copyFileToSamePartition() | ||
615 | { | 819 | { | ||
616 | const QString filePath = homeTmpDir() + "fileFromHome"; | 820 | const QString filePath = homeTmpDir() + "fileFromHome"; | ||
617 | const QString dest = homeTmpDir() + "fileFromHome_copied"; | 821 | const QString dest = homeTmpDir() + "fileFromHome_copied"; | ||
618 | createTestFile(filePath); | 822 | createTestFile(filePath); | ||
619 | copyLocalFile(filePath, dest); | 823 | copyLocalFile(filePath, dest); | ||
824 | copyLocalFileWithXattr(filePath, dest); | ||||
620 | } | 825 | } | ||
621 | 826 | | |||
622 | void JobTest::copyDirectoryToSamePartition() | 827 | void JobTest::copyDirectoryToSamePartition() | ||
I fail to understand the code here. dfaure: I fail to understand the code here.
This is the same call as two lines above, line 734, which… | |||||
623 | { | 828 | { | ||
624 | qDebug(); | 829 | qDebug(); | ||
625 | const QString src = homeTmpDir() + "dirFromHome"; | 830 | const QString src = homeTmpDir() + "dirFromHome"; | ||
626 | const QString dest = homeTmpDir() + "dirFromHome_copied"; | 831 | const QString dest = homeTmpDir() + "dirFromHome_copied"; | ||
627 | createTestDirectory(src); | 832 | createTestDirectory(src); | ||
628 | copyLocalDirectory(src, dest); | 833 | copyLocalDirectory(src, dest); | ||
834 | copyLocalDirectoryWithXattr(src, dest); | ||||
629 | } | 835 | } | ||
630 | 836 | | |||
631 | void JobTest::copyDirectoryToExistingDirectory() | 837 | void JobTest::copyDirectoryToExistingDirectory() | ||
632 | { | 838 | { | ||
633 | qDebug(); | 839 | qDebug(); | ||
634 | // just the same as copyDirectoryToSamePartition, but this time dest exists. | 840 | // just the same as copyDirectoryToSamePartition, but this time dest exists. | ||
635 | // So we get a subdir, "dirFromHome_copy/dirFromHome" | 841 | // So we get a subdir, "dirFromHome_copy/dirFromHome" | ||
636 | const QString src = homeTmpDir() + "dirFromHome"; | 842 | const QString src = homeTmpDir() + "dirFromHome"; | ||
637 | const QString dest = homeTmpDir() + "dirFromHome_copied"; | 843 | const QString dest = homeTmpDir() + "dirFromHome_copied"; | ||
638 | createTestDirectory(src); | 844 | createTestDirectory(src); | ||
639 | createTestDirectory(dest); | 845 | createTestDirectory(dest); | ||
640 | copyLocalDirectory(src, dest, AlreadyExists); | 846 | copyLocalDirectory(src, dest, AlreadyExists); | ||
847 | copyLocalDirectoryWithXattr(src, dest, AlreadyExists); | ||||
641 | } | 848 | } | ||
642 | 849 | | |||
643 | void JobTest::copyFileToOtherPartition() | 850 | void JobTest::copyFileToOtherPartition() | ||
644 | { | 851 | { | ||
645 | qDebug(); | 852 | qDebug(); | ||
646 | const QString filePath = homeTmpDir() + "fileFromHome"; | 853 | const QString filePath = homeTmpDir() + "fileFromHome"; | ||
647 | const QString dest = otherTmpDir() + "fileFromHome_copied"; | 854 | const QString dest = otherTmpDir() + "fileFromHome_copied"; | ||
648 | createTestFile(filePath); | 855 | createTestFile(filePath); | ||
649 | copyLocalFile(filePath, dest); | 856 | copyLocalFile(filePath, dest); | ||
857 | copyLocalFileWithXattr(filePath, dest); | ||||
This will set m_SkipXattr unconditionally even if the source FS does not support Xattrs. bruns: This will set `m_SkipXattr` unconditionally even if the source FS does not support Xattrs. | |||||
bruns: And now you **only** check the source FS, but no longer the destination FS. | |||||
To be honest, I was confused by your first comment. What was wrong with the first version of this code? My understanding was that we want to skip xattr stuff if either source or destination doesn't support it arrowd: To be honest, I was confused by your first comment. What was wrong with the first version of… | |||||
setXattr, called from checkXattrFsSupport, unconditionally sets m_SkipXattr. bool canRead = checkXattrFsSupport(homeDir); bool canWrite = checkXattrFsSupport(otherHomeDir); if (canRead && canWrite) { and get rid of the m_SkipXAttr variable. bruns: `setXattr`, called from `checkXattrFsSupport`, unconditionally sets `m_SkipXattr`.
You want… | |||||
650 | } | 858 | } | ||
651 | 859 | | |||
652 | void JobTest::copyDirectoryToOtherPartition() | 860 | void JobTest::copyDirectoryToOtherPartition() | ||
653 | { | 861 | { | ||
654 | qDebug(); | 862 | qDebug(); | ||
655 | const QString src = homeTmpDir() + "dirFromHome"; | 863 | const QString src = homeTmpDir() + "dirFromHome"; | ||
656 | const QString dest = otherTmpDir() + "dirFromHome_copied"; | 864 | const QString dest = otherTmpDir() + "dirFromHome_copied"; | ||
657 | createTestDirectory(src); | 865 | createTestDirectory(src); | ||
658 | copyLocalDirectory(src, dest); | 866 | copyLocalDirectory(src, dest); | ||
867 | copyLocalDirectoryWithXattr(src, dest); | ||||
659 | } | 868 | } | ||
660 | 869 | | |||
661 | void JobTest::copyRelativeSymlinkToSamePartition() // #352927 | 870 | void JobTest::copyRelativeSymlinkToSamePartition() // #352927 | ||
662 | { | 871 | { | ||
663 | #ifdef Q_OS_WIN | 872 | #ifdef Q_OS_WIN | ||
664 | QSKIP("Skipping symlink test on Windows"); | 873 | QSKIP("Skipping symlink test on Windows"); | ||
665 | #else | 874 | #else | ||
666 | const QString filePath = homeTmpDir() + "testlink"; | 875 | const QString filePath = homeTmpDir() + "testlink"; | ||
667 | const QString dest = homeTmpDir() + "testlink_copied"; | 876 | const QString dest = homeTmpDir() + "testlink_copied"; | ||
668 | createTestSymlink(filePath, "relative"); | 877 | createTestSymlink(filePath, "relative"); | ||
669 | copyLocalSymlink(filePath, dest, QStringLiteral("relative")); | 878 | copyLocalSymlink(filePath, dest, QStringLiteral("relative")); | ||
670 | QFile::remove(filePath); | 879 | QFile::remove(filePath); | ||
dfaure: please revert no-op changes | |||||
671 | #endif | 880 | #endif | ||
672 | } | 881 | } | ||
673 | 882 | | |||
674 | void JobTest::copyAbsoluteSymlinkToOtherPartition() | 883 | void JobTest::copyAbsoluteSymlinkToOtherPartition() | ||
675 | { | 884 | { | ||
676 | #ifdef Q_OS_WIN | 885 | #ifdef Q_OS_WIN | ||
677 | QSKIP("Skipping symlink test on Windows"); | 886 | QSKIP("Skipping symlink test on Windows"); | ||
678 | #else | 887 | #else | ||
679 | const QString filePath = homeTmpDir() + "testlink"; | 888 | const QString filePath = homeTmpDir() + "testlink"; | ||
680 | const QString dest = otherTmpDir() + "testlink_copied"; | 889 | const QString dest = otherTmpDir() + "testlink_copied"; | ||
681 | createTestSymlink(filePath, QFile::encodeName(homeTmpDir())); | 890 | createTestSymlink(filePath, QFile::encodeName(homeTmpDir())); | ||
dfaure: no-op (just empty lines) | |||||
682 | copyLocalSymlink(filePath, dest, homeTmpDir()); | 891 | copyLocalSymlink(filePath, dest, homeTmpDir()); | ||
683 | QFile::remove(filePath); | 892 | QFile::remove(filePath); | ||
dfaure: empty line | |||||
684 | #endif | 893 | #endif | ||
685 | } | 894 | } | ||
686 | 895 | | |||
687 | void JobTest::copyFolderWithUnaccessibleSubfolder() | 896 | void JobTest::copyFolderWithUnaccessibleSubfolder() | ||
688 | { | 897 | { | ||
689 | #ifdef Q_OS_WIN | 898 | #ifdef Q_OS_WIN | ||
690 | QSKIP("Skipping unaccessible folder test on Windows, cannot remove all permissions from a folder"); | 899 | QSKIP("Skipping unaccessible folder test on Windows, cannot remove all permissions from a folder"); | ||
691 | #endif | 900 | #endif | ||
▲ Show 20 Lines • Show All 1308 Lines • Show Last 20 Lines |
Whats the reason to split off the path?
Also, you can not just concatenate the command and args, see https://doc.qt.io/qt-5/qprocess.html#details