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