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 45 Lines • ▼ Show 20 Line(s) | 80 | { | |||
89 | cleanupTestCase(); | 90 | cleanupTestCase(); | ||
90 | homeTmpDir(); // create it | 91 | homeTmpDir(); // create it | ||
91 | if (!QFile::exists(otherTmpDir())) { | 92 | if (!QFile::exists(otherTmpDir())) { | ||
92 | bool ok = QDir().mkdir(otherTmpDir()); | 93 | bool ok = QDir().mkdir(otherTmpDir()); | ||
93 | if (!ok) { | 94 | if (!ok) { | ||
94 | qFatal("couldn't create %s", qPrintable(otherTmpDir())); | 95 | qFatal("couldn't create %s", qPrintable(otherTmpDir())); | ||
95 | } | 96 | } | ||
96 | } | 97 | } | ||
98 | | ||||
99 | /***** | ||||
100 | * Set platform xattr related commands. | ||||
101 | * Linux commands: setfattr, getfattr | ||||
102 | * BSD commands: setextattr, getextattr | ||||
103 | * MacOS commands: xattr -w, xattr -p | ||||
104 | ****/ | ||||
105 | m_getXattrCmd = QStandardPaths::findExecutable("getfattr").split("/").last(); | ||||
bruns: Whats the reason to split off the path?
Also, you can not just concatenate the command and args… | |||||
106 | if (m_getXattrCmd == "getfattr") { | ||||
107 | m_setXattrCmd = "setfattr"; | ||||
108 | } else { | ||||
109 | m_getXattrCmd = QStandardPaths::findExecutable("getextattr").split("/").last(); | ||||
110 | if (m_getXattrCmd == "getextattr") { | ||||
111 | m_setXattrCmd = "setextattr"; | ||||
112 | } else { | ||||
113 | m_getXattrCmd = QStandardPaths::findExecutable("xattr").split("/").last(); | ||||
114 | if (m_getXattrCmd == "xattr") { | ||||
115 | m_getXattrCmd += " -p"; | ||||
116 | m_setXattrCmd = "xattr -w"; | ||||
117 | } else { | ||||
118 | qWarning() << "Xatr command not foud."; | ||||
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… | |||||
119 | } | ||||
120 | } | ||||
121 | } | ||||
97 | #if 0 | 122 | #if 0 | ||
98 | if (KProtocolInfo::isKnownProtocol("system")) { | 123 | if (KProtocolInfo::isKnownProtocol("system")) { | ||
99 | if (!QFile::exists(realSystemPath())) { | 124 | if (!QFile::exists(realSystemPath())) { | ||
100 | bool ok = dir.mkdir(realSystemPath()); | 125 | bool ok = dir.mkdir(realSystemPath()); | ||
101 | if (!ok) { | 126 | if (!ok) { | ||
102 | qFatal("couldn't create %s", qPrintable(realSystemPath())); | 127 | qFatal("couldn't create %s", qPrintable(realSystemPath())); | ||
103 | } | 128 | } | ||
104 | } | 129 | } | ||
▲ Show 20 Lines • Show All 333 Lines • ▼ Show 20 Line(s) | 446 | { | |||
438 | QTimer::singleShot(200, this, [job]() { | 463 | QTimer::singleShot(200, this, [job]() { | ||
439 | job->kill(); | 464 | job->kill(); | ||
440 | }); | 465 | }); | ||
441 | 466 | | |||
442 | QTRY_VERIFY(jobFinished); | 467 | QTRY_VERIFY(jobFinished); | ||
443 | } | 468 | } | ||
444 | 469 | | |||
445 | //// | 470 | //// | ||
471 | static QHash<QString, QString> getSampleXattrs() | ||||
472 | { | ||||
473 | QHash<QString, QString> attrs; | ||||
474 | attrs["user.name with space"] = "value with spaces"; | ||||
475 | attrs["user.baloo.rating"] = "1"; | ||||
476 | attrs["user.fnewLine"] = "line1\\nline2"; | ||||
477 | attrs["user.flistNull"] = "item1\\0item2"; | ||||
478 | attrs["user.fattr.with.a.lot.of.namespaces"] = "true"; | ||||
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? | |||||
479 | attrs["user.name with space"] = "value with spaces"; | ||||
480 | return attrs; | ||||
dfaure: this method has weird indentation | |||||
481 | } | ||||
482 | | ||||
483 | void JobTest::checkXattrFsSupport(const QString &dest) | ||||
484 | { | ||||
usta: const ? | |||||
485 | QStringList path = dest.split("/"); | ||||
486 | path.removeLast(); | ||||
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… | |||||
487 | QString writeTest = path.join("/") + "/fsXattrTestFile"; | ||||
488 | createTestFile(writeTest); | ||||
489 | setXattr(writeTest); | ||||
490 | QFile::remove(writeTest); | ||||
bruns: should be `dest`. | |||||
491 | } | ||||
492 | | ||||
493 | void JobTest::setXattr(const QString &src) | ||||
494 | { | ||||
495 | // TODO: Linux (setfattr) only | ||||
496 | if (m_setXattrCmd != "setfattr") { | ||||
497 | QSKIP("Linux only test"); | ||||
498 | } | ||||
499 | | ||||
500 | QProcess xattrwriter; | ||||
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 | |||||
501 | xattrwriter.setProcessChannelMode(QProcess::MergedChannels); | ||||
502 | // arguments 0:"-n" 1:"name" 2:"-v", 3:"value" 4:src | ||||
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&)>… | |||||
503 | QStringList arguments = {"-n", "", "-v", "", "-h", src}; | ||||
504 | QHash<QString, QString> attrs = getSampleXattrs(); | ||||
505 | QHashIterator<QString, QString> i(attrs); | ||||
506 | while (i.hasNext()) { | ||||
507 | i.next(); | ||||
508 | arguments.replace(1, i.key()); | ||||
bruns: Should have a `break` or `return` here. | |||||
509 | arguments.replace(3, i.value()); | ||||
510 | xattrwriter.start(m_setXattrCmd, arguments); | ||||
511 | QVERIFY(xattrwriter.waitForStarted()); | ||||
512 | QCOMPARE(xattrwriter.state(), QProcess::Running); | ||||
bruns: This seem bogus to me - what guarantees the process has not already finished? | |||||
513 | QVERIFY(xattrwriter.waitForFinished(-1)); | ||||
514 | QCOMPARE(xattrwriter.exitStatus(), QProcess::NormalExit); | ||||
515 | QList<QByteArray> resultdest = xattrwriter.readAllStandardOutput().split('\n'); | ||||
516 | if (!resultdest[0].isEmpty()) { | ||||
517 | QWARN("Error writing user xattr. Xattr copy tests will be disabled."); | ||||
518 | qDebug() << resultdest; | ||||
519 | m_SkipXattr = true; | ||||
520 | } | ||||
521 | } | ||||
522 | } | ||||
523 | | ||||
524 | void JobTest::compareXattr(const QString &src, const QString &dest) | ||||
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 =… | |||||
525 | { | ||||
526 | // TODO: Linux (setfattr) only | ||||
527 | if (m_setXattrCmd != "setfattr") { | ||||
528 | QSKIP("Linux only test"); | ||||
529 | } | ||||
530 | | ||||
531 | QProcess xattrreader; | ||||
usta: camelCase ? xattrreader => xattrReader | |||||
usta: typo :) xattRreader => xattrReader | |||||
cochise: I really need to sleep, rs. | |||||
532 | xattrreader.setProcessChannelMode(QProcess::MergedChannels); | ||||
533 | QStringList arguments = {"-d", src}; | ||||
534 | xattrreader.start(m_getXattrCmd, arguments); | ||||
535 | QVERIFY(xattrreader.waitForStarted()); | ||||
536 | QCOMPARE(xattrreader.state(), QProcess::Running); | ||||
bruns: dito | |||||
537 | QVERIFY(xattrreader.waitForFinished(-1)); | ||||
538 | QCOMPARE(xattrreader.exitStatus(), QProcess::NormalExit); | ||||
539 | QList<QByteArray> resultsrc = xattrreader.readAllStandardOutput().split('\n'); | ||||
540 | // Line 1 is the file name | ||||
541 | resultsrc.removeAt(1); | ||||
542 | | ||||
543 | arguments.replace(1, dest); | ||||
544 | xattrreader.start(m_getXattrCmd, arguments); | ||||
545 | QVERIFY(xattrreader.waitForStarted()); | ||||
546 | QCOMPARE(xattrreader.state(), QProcess::Running); | ||||
547 | QVERIFY(xattrreader.waitForFinished(-1)); | ||||
548 | QCOMPARE(xattrreader.exitStatus(), QProcess::NormalExit); | ||||
549 | QList<QByteArray> resultdest = xattrreader.readAllStandardOutput().split('\n'); | ||||
550 | resultdest.removeAt(1); | ||||
551 | | ||||
552 | QCOMPARE(resultdest, resultsrc); | ||||
553 | } | ||||
446 | 554 | | |||
447 | void JobTest::copyLocalFile(const QString &src, const QString &dest) | 555 | void JobTest::copyLocalFile(const QString &src, const QString &dest) | ||
448 | { | 556 | { | ||
449 | const QUrl u = QUrl::fromLocalFile(src); | 557 | const QUrl u = QUrl::fromLocalFile(src); | ||
450 | const QUrl d = QUrl::fromLocalFile(dest); | 558 | const QUrl d = QUrl::fromLocalFile(dest); | ||
451 | 559 | | |||
452 | const int perms = 0666; | 560 | const int perms = 0666; | ||
453 | // copy the file with file_copy | 561 | // copy the file with file_copy | ||
pino: extra empty line | |||||
454 | KIO::Job *job = KIO::file_copy(u, d, perms, KIO::HideProgressInfo); | 562 | KIO::Job *job = KIO::file_copy(u, d, perms, KIO::HideProgressInfo); | ||
455 | job->setUiDelegate(nullptr); | 563 | job->setUiDelegate(nullptr); | ||
456 | bool ok = job->exec(); | 564 | bool ok = job->exec(); | ||
457 | QVERIFY(ok); | 565 | QVERIFY(ok); | ||
458 | QVERIFY(QFile::exists(dest)); | 566 | QVERIFY(QFile::exists(dest)); | ||
459 | QVERIFY(QFile::exists(src)); // still there | 567 | QVERIFY(QFile::exists(src)); // still there | ||
460 | QCOMPARE(int(QFileInfo(dest).permissions()), int(0x6666)); | 568 | QCOMPARE(int(QFileInfo(dest).permissions()), int(0x6666)); | ||
569 | compareXattr(src, dest); | ||||
461 | 570 | | |||
462 | { | 571 | { | ||
463 | // check that the timestamp is the same (#24443) | 572 | // check that the timestamp is the same (#24443) | ||
464 | // Note: this only works because of copy() in kio_file. | 573 | // Note: this only works because of copy() in kio_file. | ||
465 | // The datapump solution ignores mtime, the app has to call FileCopyJob::setModificationTime() | 574 | // The datapump solution ignores mtime, the app has to call FileCopyJob::setModificationTime() | ||
466 | QFileInfo srcInfo(src); | 575 | QFileInfo srcInfo(src); | ||
467 | QFileInfo destInfo(dest); | 576 | QFileInfo destInfo(dest); | ||
468 | #ifdef Q_OS_WIN | 577 | #ifdef Q_OS_WIN | ||
Show All 10 Lines | 583 | #endif | |||
479 | job = KIO::copy(u, d, KIO::HideProgressInfo); | 588 | job = KIO::copy(u, d, KIO::HideProgressInfo); | ||
480 | QSignalSpy spyCopyingDone(job, SIGNAL(copyingDone(KIO::Job*,QUrl,QUrl,QDateTime,bool,bool))); | 589 | QSignalSpy spyCopyingDone(job, SIGNAL(copyingDone(KIO::Job*,QUrl,QUrl,QDateTime,bool,bool))); | ||
481 | job->setUiDelegate(nullptr); | 590 | job->setUiDelegate(nullptr); | ||
482 | job->setUiDelegateExtension(nullptr); | 591 | job->setUiDelegateExtension(nullptr); | ||
483 | ok = job->exec(); | 592 | ok = job->exec(); | ||
484 | QVERIFY(ok); | 593 | QVERIFY(ok); | ||
485 | QVERIFY(QFile::exists(dest)); | 594 | QVERIFY(QFile::exists(dest)); | ||
486 | QVERIFY(QFile::exists(src)); // still there | 595 | QVERIFY(QFile::exists(src)); // still there | ||
596 | compareXattr(src, dest); | ||||
487 | { | 597 | { | ||
488 | // check that the timestamp is the same (#24443) | 598 | // check that the timestamp is the same (#24443) | ||
489 | QFileInfo srcInfo(src); | 599 | QFileInfo srcInfo(src); | ||
490 | QFileInfo destInfo(dest); | 600 | QFileInfo destInfo(dest); | ||
491 | #ifdef Q_OS_WIN | 601 | #ifdef Q_OS_WIN | ||
492 | // win32 time may differs in msec part | 602 | // win32 time may differs in msec part | ||
493 | QCOMPARE(srcInfo.lastModified().toString("dd.MM.yyyy hh:mm"), | 603 | QCOMPARE(srcInfo.lastModified().toString("dd.MM.yyyy hh:mm"), | ||
494 | destInfo.lastModified().toString("dd.MM.yyyy hh:mm")); | 604 | destInfo.lastModified().toString("dd.MM.yyyy hh:mm")); | ||
495 | #else | 605 | #else | ||
496 | QCOMPARE(srcInfo.lastModified(), destInfo.lastModified()); | 606 | QCOMPARE(srcInfo.lastModified(), destInfo.lastModified()); | ||
497 | #endif | 607 | #endif | ||
498 | } | 608 | } | ||
499 | QCOMPARE(spyCopyingDone.count(), 1); | 609 | QCOMPARE(spyCopyingDone.count(), 1); | ||
500 | 610 | | |||
501 | // cleanup and retry with KIO::copyAs() | 611 | // cleanup and retry with KIO::copyAs() | ||
502 | QFile::remove(dest); | 612 | QFile::remove(dest); | ||
503 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | 613 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | ||
504 | job->setUiDelegate(nullptr); | 614 | job->setUiDelegate(nullptr); | ||
505 | job->setUiDelegateExtension(nullptr); | 615 | job->setUiDelegateExtension(nullptr); | ||
506 | QVERIFY(job->exec()); | 616 | QVERIFY(job->exec()); | ||
507 | QVERIFY(QFile::exists(dest)); | 617 | QVERIFY(QFile::exists(dest)); | ||
508 | QVERIFY(QFile::exists(src)); // still there | 618 | QVERIFY(QFile::exists(src)); // still there | ||
619 | compareXattr(src, dest); | ||||
509 | 620 | | |||
510 | // Do it again, with Overwrite. | 621 | // Do it again, with Overwrite. | ||
511 | job = KIO::copyAs(u, d, KIO::Overwrite | KIO::HideProgressInfo); | 622 | job = KIO::copyAs(u, d, KIO::Overwrite | KIO::HideProgressInfo); | ||
512 | job->setUiDelegate(nullptr); | 623 | job->setUiDelegate(nullptr); | ||
513 | job->setUiDelegateExtension(nullptr); | 624 | job->setUiDelegateExtension(nullptr); | ||
514 | QVERIFY(job->exec()); | 625 | QVERIFY(job->exec()); | ||
515 | QVERIFY(QFile::exists(dest)); | 626 | QVERIFY(QFile::exists(dest)); | ||
516 | QVERIFY(QFile::exists(src)); // still there | 627 | QVERIFY(QFile::exists(src)); // still there | ||
628 | compareXattr(src, dest); | ||||
517 | 629 | | |||
518 | // Do it again, without Overwrite (should fail). | 630 | // Do it again, without Overwrite (should fail). | ||
519 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | 631 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | ||
520 | job->setUiDelegate(nullptr); | 632 | job->setUiDelegate(nullptr); | ||
521 | job->setUiDelegateExtension(nullptr); | 633 | job->setUiDelegateExtension(nullptr); | ||
522 | QVERIFY(!job->exec()); | 634 | QVERIFY(!job->exec()); | ||
523 | 635 | | |||
524 | // Clean up | 636 | // Clean up | ||
637 | QFile::remove(src); | ||||
525 | QFile::remove(dest); | 638 | QFile::remove(dest); | ||
526 | } | 639 | } | ||
527 | 640 | | |||
528 | void JobTest::copyLocalDirectory(const QString &src, const QString &_dest, int flags) | 641 | void JobTest::copyLocalDirectory(const QString &src, const QString &_dest, int flags) | ||
dfaure: prepend `static` | |||||
529 | { | 642 | { | ||
530 | QVERIFY(QFileInfo(src).isDir()); | 643 | QVERIFY(QFileInfo(src).isDir()); | ||
531 | QVERIFY(QFileInfo(src + "/testfile").isFile()); | 644 | QVERIFY(QFileInfo(src + "/testfile").isFile()); | ||
532 | QUrl u = QUrl::fromLocalFile(src); | 645 | QUrl u = QUrl::fromLocalFile(src); | ||
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… | |||||
533 | QString dest(_dest); | 646 | QString dest(_dest); | ||
534 | QUrl d = QUrl::fromLocalFile(dest); | 647 | QUrl d = QUrl::fromLocalFile(dest); | ||
dfaure: QVERIFY(....) around all calls to waitForFinished
(repeats) | |||||
535 | if (flags & AlreadyExists) { | 648 | if (flags & AlreadyExists) { | ||
536 | QVERIFY(QFile::exists(dest)); | 649 | QVERIFY(QFile::exists(dest)); | ||
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… | |||||
537 | } else { | 650 | } else { | ||
538 | QVERIFY(!QFile::exists(dest)); | 651 | QVERIFY(!QFile::exists(dest)); | ||
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 | |||||
539 | } | 652 | } | ||
pino: check using `QVERIFY`/`QCOMPARE` that the process ended correctly | |||||
540 | 653 | | |||
541 | KIO::Job *job = KIO::copy(u, d, KIO::HideProgressInfo); | 654 | KIO::Job *job = KIO::copy(u, d, KIO::HideProgressInfo); | ||
542 | job->setUiDelegate(nullptr); | 655 | job->setUiDelegate(nullptr); | ||
543 | job->setUiDelegateExtension(nullptr); | 656 | job->setUiDelegateExtension(nullptr); | ||
544 | bool ok = job->exec(); | 657 | bool ok = job->exec(); | ||
545 | QVERIFY(ok); | 658 | QVERIFY(ok); | ||
546 | QVERIFY(QFile::exists(dest)); | 659 | QVERIFY(QFile::exists(dest)); | ||
547 | QVERIFY(QFileInfo(dest).isDir()); | 660 | QVERIFY(QFileInfo(dest).isDir()); | ||
548 | QVERIFY(QFileInfo(dest + "/testfile").isFile()); | 661 | QVERIFY(QFileInfo(dest + "/testfile").isFile()); | ||
549 | QVERIFY(QFile::exists(src)); // still there | 662 | QVERIFY(QFile::exists(src)); // still there | ||
550 | 663 | | |||
551 | if (flags & AlreadyExists) { | 664 | if (flags & AlreadyExists) { | ||
552 | dest += '/' + u.fileName(); | 665 | dest += '/' + u.fileName(); | ||
553 | //qDebug() << "Expecting dest=" << dest; | 666 | //qDebug() << "Expecting dest=" << dest; | ||
554 | } | 667 | } | ||
555 | 668 | | |||
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… | |||||
556 | // CopyJob::setNextDirAttribute isn't implemented for Windows currently. | 669 | // CopyJob::setNextDirAttribute isn't implemented for Windows currently. | ||
557 | #ifndef Q_OS_WIN | 670 | #ifndef Q_OS_WIN | ||
558 | { | 671 | { | ||
559 | // Check that the timestamp is the same (#24443) | 672 | // Check that the timestamp is the same (#24443) | ||
560 | QFileInfo srcInfo(src); | 673 | QFileInfo srcInfo(src); | ||
561 | QFileInfo destInfo(dest); | 674 | QFileInfo destInfo(dest); | ||
562 | QCOMPARE(srcInfo.lastModified(), destInfo.lastModified()); | 675 | QCOMPARE(srcInfo.lastModified(), destInfo.lastModified()); | ||
563 | } | 676 | } | ||
564 | #endif | 677 | #endif | ||
565 | 678 | | |||
566 | // Do it again, with Overwrite. | 679 | // Do it again, with Overwrite. | ||
567 | // Use copyAs, we don't want a subdir inside d. | 680 | // Use copyAs, we don't want a subdir inside d. | ||
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 | |||||
568 | job = KIO::copyAs(u, d, KIO::HideProgressInfo | KIO::Overwrite); | 681 | job = KIO::copyAs(u, d, KIO::HideProgressInfo | KIO::Overwrite); | ||
569 | job->setUiDelegate(nullptr); | 682 | job->setUiDelegate(nullptr); | ||
570 | job->setUiDelegateExtension(nullptr); | 683 | job->setUiDelegateExtension(nullptr); | ||
571 | ok = job->exec(); | 684 | ok = job->exec(); | ||
572 | QVERIFY(ok); | 685 | QVERIFY(ok); | ||
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. =] | |||||
573 | 686 | | |||
574 | // Do it again, without Overwrite (should fail). | 687 | // Do it again, without Overwrite (should fail). | ||
575 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | 688 | job = KIO::copyAs(u, d, KIO::HideProgressInfo); | ||
576 | job->setUiDelegate(nullptr); | 689 | job->setUiDelegate(nullptr); | ||
577 | job->setUiDelegateExtension(nullptr); | 690 | job->setUiDelegateExtension(nullptr); | ||
578 | ok = job->exec(); | 691 | ok = job->exec(); | ||
579 | QVERIFY(!ok); | 692 | QVERIFY(!ok); | ||
580 | } | 693 | } | ||
581 | 694 | | |||
582 | #ifndef Q_OS_WIN | 695 | #ifndef Q_OS_WIN | ||
583 | static QString linkTarget(const QString &path) | 696 | static QString linkTarget(const QString &path) | ||
584 | { | 697 | { | ||
585 | // Use readlink on Unix because symLinkTarget turns relative targets into absolute (#352927) | 698 | // Use readlink on Unix because symLinkTarget turns relative targets into absolute (#352927) | ||
586 | char linkTargetBuffer[4096]; | 699 | char linkTargetBuffer[4096]; | ||
587 | const int n = readlink(QFile::encodeName(path).constData(), linkTargetBuffer, sizeof(linkTargetBuffer) - 1); | 700 | const int n = readlink(QFile::encodeName(path).constData(), linkTargetBuffer, sizeof(linkTargetBuffer) - 1); | ||
588 | if (n != -1) { | 701 | if (n != -1) { | ||
dfaure: QSKIP | |||||
589 | linkTargetBuffer[n] = 0; | 702 | linkTargetBuffer[n] = 0; | ||
590 | } | 703 | } | ||
591 | return QFile::decodeName(linkTargetBuffer); | 704 | return QFile::decodeName(linkTargetBuffer); | ||
592 | } | 705 | } | ||
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' | |||||
593 | 706 | | |||
594 | static void copyLocalSymlink(const QString &src, const QString &dest, const QString &expectedLinkTarget) | 707 | static void copyLocalSymlink(const QString &src, const QString &dest, const QString &expectedLinkTarget) | ||
595 | { | 708 | { | ||
596 | QT_STATBUF buf; | 709 | QT_STATBUF buf; | ||
597 | QVERIFY(QT_LSTAT(QFile::encodeName(src).constData(), &buf) == 0); | 710 | QVERIFY(QT_LSTAT(QFile::encodeName(src).constData(), &buf) == 0); | ||
598 | QUrl u = QUrl::fromLocalFile(src); | 711 | QUrl u = QUrl::fromLocalFile(src); | ||
599 | QUrl d = QUrl::fromLocalFile(dest); | 712 | QUrl d = QUrl::fromLocalFile(dest); | ||
600 | 713 | | |||
601 | // copy the symlink | 714 | // copy the symlink | ||
602 | KIO::Job *job = KIO::copy(u, d, KIO::HideProgressInfo); | 715 | KIO::Job *job = KIO::copy(u, d, KIO::HideProgressInfo); | ||
dfaure: Please extract a function from those 12 lines, which are duplicated. | |||||
603 | job->setUiDelegate(nullptr); | 716 | job->setUiDelegate(nullptr); | ||
604 | job->setUiDelegateExtension(nullptr); | 717 | job->setUiDelegateExtension(nullptr); | ||
dfaure: const | |||||
605 | QVERIFY2(job->exec(), qPrintable(QString::number(job->error()))); | 718 | QVERIFY2(job->exec(), qPrintable(QString::number(job->error()))); | ||
606 | QVERIFY(QT_LSTAT(QFile::encodeName(dest).constData(), &buf) == 0); // dest exists | 719 | QVERIFY(QT_LSTAT(QFile::encodeName(dest).constData(), &buf) == 0); // dest exists | ||
dfaure: const | |||||
607 | QCOMPARE(linkTarget(dest), expectedLinkTarget); | 720 | QCOMPARE(linkTarget(dest), expectedLinkTarget); | ||
608 | 721 | | |||
609 | // cleanup | 722 | // cleanup | ||
610 | QFile::remove(dest); | 723 | QFile::remove(dest); | ||
611 | } | 724 | } | ||
612 | #endif | 725 | #endif | ||
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… | |||||
613 | 726 | | |||
614 | void JobTest::copyFileToSamePartition() | 727 | void JobTest::copyFileToSamePartition() | ||
615 | { | 728 | { | ||
616 | const QString filePath = homeTmpDir() + "fileFromHome"; | 729 | const QString filePath = homeTmpDir() + "fileFromHome"; | ||
617 | const QString dest = homeTmpDir() + "fileFromHome_copied"; | 730 | const QString dest = homeTmpDir() + "fileFromHome_copied"; | ||
618 | createTestFile(filePath); | 731 | createTestFile(filePath); | ||
732 | checkXattrFsSupport(filePath); | ||||
733 | if (!m_SkipXattr) { | ||||
734 | setXattr(filePath); | ||||
735 | } | ||||
736 | setXattr(filePath); | ||||
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… | |||||
619 | copyLocalFile(filePath, dest); | 737 | copyLocalFile(filePath, dest); | ||
620 | } | 738 | } | ||
dfaure: prepend `static` | |||||
bruns: Do the lookup once in `JobTest::initTestCase` | |||||
bruns: Check for both set and get commands. | |||||
621 | 739 | | |||
622 | void JobTest::copyDirectoryToSamePartition() | 740 | void JobTest::copyDirectoryToSamePartition() | ||
623 | { | 741 | { | ||
624 | qDebug(); | 742 | qDebug(); | ||
625 | const QString src = homeTmpDir() + "dirFromHome"; | 743 | const QString src = homeTmpDir() + "dirFromHome"; | ||
626 | const QString dest = homeTmpDir() + "dirFromHome_copied"; | 744 | const QString dest = homeTmpDir() + "dirFromHome_copied"; | ||
627 | createTestDirectory(src); | 745 | createTestDirectory(src); | ||
628 | copyLocalDirectory(src, dest); | 746 | copyLocalDirectory(src, dest); | ||
629 | } | 747 | } | ||
630 | 748 | | |||
631 | void JobTest::copyDirectoryToExistingDirectory() | 749 | void JobTest::copyDirectoryToExistingDirectory() | ||
632 | { | 750 | { | ||
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. | |||||
633 | qDebug(); | 751 | qDebug(); | ||
634 | // just the same as copyDirectoryToSamePartition, but this time dest exists. | 752 | // just the same as copyDirectoryToSamePartition, but this time dest exists. | ||
635 | // So we get a subdir, "dirFromHome_copy/dirFromHome" | 753 | // So we get a subdir, "dirFromHome_copy/dirFromHome" | ||
bruns: Keep the full path. | |||||
636 | const QString src = homeTmpDir() + "dirFromHome"; | 754 | const QString src = homeTmpDir() + "dirFromHome"; | ||
637 | const QString dest = homeTmpDir() + "dirFromHome_copied"; | 755 | const QString dest = homeTmpDir() + "dirFromHome_copied"; | ||
638 | createTestDirectory(src); | 756 | createTestDirectory(src); | ||
639 | createTestDirectory(dest); | 757 | createTestDirectory(dest); | ||
640 | copyLocalDirectory(src, dest, AlreadyExists); | 758 | copyLocalDirectory(src, dest, AlreadyExists); | ||
641 | } | 759 | } | ||
642 | 760 | | |||
643 | void JobTest::copyFileToOtherPartition() | 761 | void JobTest::copyFileToOtherPartition() | ||
644 | { | 762 | { | ||
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. | |||||
645 | qDebug(); | 763 | qDebug(); | ||
646 | const QString filePath = homeTmpDir() + "fileFromHome"; | 764 | const QString filePath = homeTmpDir() + "fileFromHome"; | ||
647 | const QString dest = otherTmpDir() + "fileFromHome_copied"; | 765 | const QString dest = otherTmpDir() + "fileFromHome_copied"; | ||
648 | createTestFile(filePath); | 766 | createTestFile(filePath); | ||
767 | checkXattrFsSupport(filePath); | ||||
768 | checkXattrFsSupport(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… | |||||
769 | if (!m_SkipXattr) { | ||||
770 | setXattr(filePath); | ||||
771 | } | ||||
649 | copyLocalFile(filePath, dest); | 772 | copyLocalFile(filePath, dest); | ||
dfaure: always use QVERIFY2(job->exec(), qPrintable(job->errorString()));
(repeats) | |||||
650 | } | 773 | } | ||
651 | 774 | | |||
652 | void JobTest::copyDirectoryToOtherPartition() | 775 | void JobTest::copyDirectoryToOtherPartition() | ||
653 | { | 776 | { | ||
654 | qDebug(); | 777 | qDebug(); | ||
655 | const QString src = homeTmpDir() + "dirFromHome"; | 778 | const QString src = homeTmpDir() + "dirFromHome"; | ||
656 | const QString dest = otherTmpDir() + "dirFromHome_copied"; | 779 | const QString dest = otherTmpDir() + "dirFromHome_copied"; | ||
657 | createTestDirectory(src); | 780 | createTestDirectory(src); | ||
658 | copyLocalDirectory(src, dest); | 781 | copyLocalDirectory(src, dest); | ||
659 | } | 782 | } | ||
660 | 783 | | |||
661 | void JobTest::copyRelativeSymlinkToSamePartition() // #352927 | 784 | void JobTest::copyRelativeSymlinkToSamePartition() // #352927 | ||
662 | { | 785 | { | ||
663 | #ifdef Q_OS_WIN | 786 | #ifdef Q_OS_WIN | ||
664 | QSKIP("Skipping symlink test on Windows"); | 787 | QSKIP("Skipping symlink test on Windows"); | ||
665 | #else | 788 | #else | ||
666 | const QString filePath = homeTmpDir() + "testlink"; | 789 | const QString filePath = homeTmpDir() + "testlink"; | ||
667 | const QString dest = homeTmpDir() + "testlink_copied"; | 790 | const QString dest = homeTmpDir() + "testlink_copied"; | ||
668 | createTestSymlink(filePath, "relative"); | 791 | createTestSymlink(filePath, "relative"); | ||
669 | copyLocalSymlink(filePath, dest, QStringLiteral("relative")); | 792 | copyLocalSymlink(filePath, dest, QStringLiteral("relative")); | ||
793 | | ||||
dfaure: please revert no-op changes | |||||
670 | QFile::remove(filePath); | 794 | QFile::remove(filePath); | ||
671 | #endif | 795 | #endif | ||
672 | } | 796 | } | ||
673 | 797 | | |||
674 | void JobTest::copyAbsoluteSymlinkToOtherPartition() | 798 | void JobTest::copyAbsoluteSymlinkToOtherPartition() | ||
675 | { | 799 | { | ||
bruns: If this copy never happened, the check would still succeed. | |||||
676 | #ifdef Q_OS_WIN | 800 | #ifdef Q_OS_WIN | ||
677 | QSKIP("Skipping symlink test on Windows"); | 801 | QSKIP("Skipping symlink test on Windows"); | ||
678 | #else | 802 | #else | ||
679 | const QString filePath = homeTmpDir() + "testlink"; | 803 | const QString filePath = homeTmpDir() + "testlink"; | ||
680 | const QString dest = otherTmpDir() + "testlink_copied"; | 804 | const QString dest = otherTmpDir() + "testlink_copied"; | ||
805 | | ||||
dfaure: no-op (just empty lines) | |||||
681 | createTestSymlink(filePath, QFile::encodeName(homeTmpDir())); | 806 | createTestSymlink(filePath, QFile::encodeName(homeTmpDir())); | ||
682 | copyLocalSymlink(filePath, dest, homeTmpDir()); | 807 | copyLocalSymlink(filePath, dest, homeTmpDir()); | ||
808 | | ||||
dfaure: empty line | |||||
683 | QFile::remove(filePath); | 809 | QFile::remove(filePath); | ||
684 | #endif | 810 | #endif | ||
685 | } | 811 | } | ||
686 | 812 | | |||
687 | void JobTest::copyFolderWithUnaccessibleSubfolder() | 813 | void JobTest::copyFolderWithUnaccessibleSubfolder() | ||
688 | { | 814 | { | ||
689 | #ifdef Q_OS_WIN | 815 | #ifdef Q_OS_WIN | ||
690 | QSKIP("Skipping unaccessible folder test on Windows, cannot remove all permissions from a folder"); | 816 | QSKIP("Skipping unaccessible folder test on Windows, cannot remove all permissions from a folder"); | ||
▲ Show 20 Lines • Show All 1309 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