Differential D12028 Diff 31968 autotests/taglibextractortest.cpp

Changeset View

Standalone View

View Options

autotests/taglibextractortest.cpp

Show All 15 Lines
16 * You should have received a copy of the GNU Lesser General Public 16​ * You should have received a copy of the GNU Lesser General Public
17 * License along with this library; if not, write to the Free Software 17​ * License along with this library; if not, write to the Free Software
18 * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA 18​ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
19 * 19​ *
20 */ 20​ */
21 21​
22#include "taglibextractortest.h" 22​#include "taglibextractortest.h"
23#include "simpleextractionresult.h" 23​#include "simpleextractionresult.h"
24​#include "propertyinfo.h"
25​//TODO: use QTESTFINDDATA and remove this
24#include "indexerextractortestsconfig.h" 26​#include "indexerextractortestsconfig.h"
25#include "extractors/taglibextractor.h" 27​#include "extractors/taglibextractor.h"
26 28​
27#include <QDebug> 29​#include <QDebug>
28#include <QTest> 30​#include <QTest>
29#include <QDir> 31​#include <QDir>
30 32​
33​Q_DECLARE_METATYPE(KFileMetaData::Property::Property)
34​
31using namespace KFileMetaData; 35​using namespace KFileMetaData;
32 36​
33QString TagLibExtractorTest::testFilePath(const QString& fileName) const 37​QString TagLibExtractorTest::testFilePath(const QString& fileName) const
34{ 38​{
35 return QLatin1String(INDEXER_TESTS_SAMPLE_FILES_PATH) + QDir::separator() + fileName; 39​ return QLatin1String(INDEXER_TESTS_SAMPLE_FILES_PATH) + QDir::separator() + fileName;
36} 40​}
37 41​
42​const QStringList TagLibExtractorTest::propertyEnumNames(const QList<KFileMetaData::Property::Property>& keys) const
brunsUnsubmitted
Not Done

Nitpick - needs some indentation

bruns: Nitpick - needs some indentation
43​{
44​ QStringList result;
45​ for (auto key : keys) {
46​ result.append(PropertyInfo(key).name());
47​ }
48​ return result;
49​}
50​
38void TagLibExtractorTest::test() 51​void TagLibExtractorTest::test()
39{ 52​{
40 QScopedPointer<ExtractorPlugin> plugin(new TagLibExtractor(this)); 53​ QScopedPointer<ExtractorPlugin> plugin(new TagLibExtractor(this));
41 54​
42 SimpleExtractionResult resultOpus(testFilePath("test.opus"), "audio/opus"); 55​ SimpleExtractionResult resultOpus(testFilePath("test.opus"), "audio/opus");
43 plugin->extract(&resultOpus); 56​ plugin->extract(&resultOpus);
44 57​
45 QCOMPARE(resultOpus.types().size(), 1); 58​ QCOMPARE(resultOpus.types().size(), 1);
▲ Show 20 Lines • Show All 97 Lines • ▼ Show 20 Line(s)
143 QCOMPARE(resultMp4.properties().value(Property::Comment), QVariant(QStringLiteral("Comment"))); 156​ QCOMPARE(resultMp4.properties().value(Property::Comment), QVariant(QStringLiteral("Comment")));
144 QCOMPARE(resultMp4.properties().value(Property::Composer), QVariant(QStringLiteral("Composer"))); 157​ QCOMPARE(resultMp4.properties().value(Property::Composer), QVariant(QStringLiteral("Composer")));
145 QCOMPARE(resultMp4.properties().value(Property::TrackNumber).toInt(), 1); 158​ QCOMPARE(resultMp4.properties().value(Property::TrackNumber).toInt(), 1);
146 QCOMPARE(resultMp4.properties().value(Property::ReleaseYear).toInt(), 2015); 159​ QCOMPARE(resultMp4.properties().value(Property::ReleaseYear).toInt(), 2015);
147 QCOMPARE(resultMp4.properties().value(Property::Channels).toInt(), 2); 160​ QCOMPARE(resultMp4.properties().value(Property::Channels).toInt(), 2);
148 QCOMPARE(resultMp4.properties().value(Property::DiscNumber).toInt(), 1); 161​ QCOMPARE(resultMp4.properties().value(Property::DiscNumber).toInt(), 1);
149} 162​}
150 163​
164​void TagLibExtractorTest::testNoMetadata_data()
165​{
166​ const auto expectedKeys = QList<Property::Property>{
167​ Property::BitRate,
168​ Property::Channels,
169​ Property::Duration,
170​ Property::SampleRate,
171​ };
172​
173​ QTest::addColumn<QString>("path");
mgallienUnsubmitted
Done

Please remove failMessage and use directly the expected value in each addRow. I fail to see what we gain by using this variable.

mgallien: Please remove failMessage and use directly the expected value in each addRow. I fail to see…
174​ QTest::addColumn<QString>("mimeType");
175​ QTest::addColumn<QList<Property::Property>>("expectedKeys");
176​ QTest::addColumn<QString>("failMessage");
177​
178​ QTest::addRow("mp3")
179​ << QFINDTESTDATA("samplefiles/no-meta/test.mp3")
180​ << QStringLiteral("audio/mp3")
181​ << expectedKeys << QStringLiteral("Excess properties")
182​ ;
183​
184​ QTest::addRow("m4a")
185​ << QFINDTESTDATA("samplefiles/no-meta/test.m4a")
186​ << QStringLiteral("audio/mp4")
187​ << expectedKeys << QString()
188​ ;
189​
190​ QTest::addRow("flac")
191​ << QFINDTESTDATA("samplefiles/no-meta/test.flac")
192​ << QStringLiteral("audio/flac")
193​ << expectedKeys << QString()
194​ ;
195​
196​ QTest::addRow("opus")
197​ << QFINDTESTDATA("samplefiles/no-meta/test.opus")
198​ << QStringLiteral("audio/opus")
199​ << expectedKeys << QString()
200​ ;
201​
202​ QTest::addRow("ogg")
203​ << QFINDTESTDATA("samplefiles/no-meta/test.ogg")
204​ << QStringLiteral("audio/ogg")
205​ << expectedKeys << QString()
206​ ;
207​
208​ QTest::addRow("mpc")
209​ << QFINDTESTDATA("samplefiles/no-meta/test.mpc")
210​ << QStringLiteral("audio/x-musepack")
211​ << expectedKeys << QStringLiteral("Excess properties")
212​ ;
213​
214​}
215​
216​void TagLibExtractorTest::testNoMetadata()
217​{
218​ QFETCH(QString, path);
219​ QFETCH(QString, mimeType);
220​ QFETCH(QList<Property::Property>, expectedKeys);
221​ QFETCH(QString, failMessage);
222​
223​ TagLibExtractor plugin{this};
224​ SimpleExtractionResult extracted(path, mimeType);
225​ plugin.extract(&extracted);
226​
227​ const auto resultList = extracted.properties();
228​ const auto resultKeys = resultList.uniqueKeys();
229​
230​ if (!failMessage.isEmpty()) {
231​ auto excessKeys = resultKeys.toSet() - expectedKeys.toSet();
mgallienUnsubmitted
Not Done

Why not use a simple variable here ?
TagLibExtractor plugin;

mgallien: Why not use a simple variable here ? TagLibExtractor plugin;
michaelhAuthorUnsubmitted
Not Done

This converts the enums to strings. That way it is easier to spot which properties are responsible for the failure.

michaelh: This converts the enums to strings. That way it is easier to spot which properties are…
brunsUnsubmitted
Not Done

What he likely meant:
TagLibExtractor plugin{this};
instead of
QScopedPointer<ExtractorPlugin> plugin(new TagLibExtractor(this));

bruns: What he likely meant: `TagLibExtractor plugin{this};` instead of…
michaelhAuthorUnsubmitted
Not Done

Well,... In that case: D12145

michaelh: Well,... In that case: D12145
232​ const auto message = QStringLiteral("%1: %2")
233​ .arg(failMessage)
234​ .arg(propertyEnumNames(excessKeys.toList()).join(QLatin1String(", ")));
235​ QEXPECT_FAIL("", qPrintable(message), Continue);
236​ }
237​ QCOMPARE(resultKeys, expectedKeys);
238​}
239​
151QTEST_GUILESS_MAIN(TagLibExtractorTest) 240​QTEST_GUILESS_MAIN(TagLibExtractorTest)
brunsUnsubmitted
Not Done

Unfortunately QCOMPARE does not do a deep compare if sizes mismatch.

To get a better output in case the test fails, you can do something like:

auto excessKeys() = ...
auto missingKeys() = ...

if (excessKeys().size()) {
    QWARN("Excess properties: " + excessKeys.join(", "));
    if (!failMessage.isEmpty())
        QEXPECT_FAIL(...)
    QCOMPARE(excessKeys.size(), 0);
}

if (missingKeys().size()) {
    ...
}
bruns: Unfortunately QCOMPARE does not do a deep compare if sizes mismatch. To get a better output in…