Differential D15714 Diff 42270 autotests/taglibwritertest.cpp

Changeset View

Standalone View

View Options

autotests/taglibwritertest.cpp

Show All 21 Lines
22{ 22​{
23 return QLatin1String(INDEXER_TESTS_SAMPLE_FILES_PATH) + QDir::separator() + fileName; 23​ return QLatin1String(INDEXER_TESTS_SAMPLE_FILES_PATH) + QDir::separator() + fileName;
24} 24​}
25 25​
26void TagLibWriterTest::testCommonData() 26​void TagLibWriterTest::testCommonData()
27{ 27​{
28 QFETCH(QString, fileType); 28​ QFETCH(QString, fileType);
29 QFETCH(QString, mimeType); 29​ QFETCH(QString, mimeType);
30​ QFETCH(QString, stringSuffix);
30 31​
31 QString temporaryFileName = QStringLiteral("writertest.") + fileType; 32​ QString temporaryFileName = QStringLiteral("writertest.") + fileType;
32 33​
33 QFile::copy(testFilePath("test." + fileType), testFilePath(temporaryFileName)); 34​ QFile::copy(testFilePath("test." + fileType), testFilePath(temporaryFileName));
34 TagLibWriter writerPlugin{this}; 35​ TagLibWriter writerPlugin{this};
35 QCOMPARE(writerPlugin.writeMimetypes().contains(mimeType),true); 36​ QCOMPARE(writerPlugin.writeMimetypes().contains(mimeType),true);
36 37​
37 WriteData data(testFilePath(temporaryFileName), mimeType); 38​ WriteData data(testFilePath(temporaryFileName), mimeType);
38 data.add(Property::Title, QStringLiteral("Title1")); 39​
39 data.add(Property::Artist, QStringLiteral("Artist1")); 40​ data.add(Property::Title, QString(QStringLiteral("Title1") + stringSuffix));
40 data.add(Property::Album, QStringLiteral("Album1")); 41​ data.add(Property::Artist, QString(QStringLiteral("Artist1") + stringSuffix));
42​ data.add(Property::Album, QString(QStringLiteral("Album1") + stringSuffix));
41 data.add(Property::TrackNumber, 10); 43​ data.add(Property::TrackNumber, 10);
42 data.add(Property::ReleaseYear, 1999); 44​ data.add(Property::ReleaseYear, 1999);
43 data.add(Property::Genre, QStringLiteral("Genre1")); 45​ data.add(Property::Genre, QString(QStringLiteral("Genre1") + stringSuffix));
44 data.add(Property::Comment, QStringLiteral("Comment1")); 46​ data.add(Property::Comment, QString(QStringLiteral("Comment1") + stringSuffix));
45 writerPlugin.write(data); 47​ writerPlugin.write(data);
46 48​
47 TagLib::FileRef file(testFilePath(temporaryFileName).toUtf8().constData(), true); 49​ TagLib::FileRef file(testFilePath(temporaryFileName).toUtf8().constData(), true);
48 TagLib::Tag* tags = file.tag(); 50​ TagLib::Tag* tags = file.tag();
49 51​
50 QString extractedTitle = t2q(tags->title()); 52​ QString extractedTitle = t2q(tags->title());
51 QString extractedArtist = t2q(tags->artist()); 53​ QString extractedArtist = t2q(tags->artist());
52 QString extractedAlbum = t2q(tags->album()); 54​ QString extractedAlbum = t2q(tags->album());
53 uint extractedTrackNumber = tags->track(); 55​ uint extractedTrackNumber = tags->track();
54 uint extractedYear = tags->year(); 56​ uint extractedYear = tags->year();
55 QString extractedGenre = t2q(tags->genre()); 57​ QString extractedGenre = t2q(tags->genre());
56 QString extractedComment = t2q(tags->comment()); 58​ QString extractedComment = t2q(tags->comment());
57 59​
58 QCOMPARE(extractedTitle, QStringLiteral("Title1")); 60​ QCOMPARE(extractedTitle, QString(QStringLiteral("Title1") + stringSuffix.toUtf8()));
brunsUnsubmitted
Done

I don't think it is a good idea to do a stringSuffix.toUtf8().fromUtf8() round trip here for each test case and for each tag.

bruns: I don't think it is a good idea to do a `stringSuffix.toUtf8().fromUtf8()` round trip here for…
59 QCOMPARE(extractedArtist, QStringLiteral("Artist1")); 61​ QCOMPARE(extractedArtist, QString(QStringLiteral("Artist1") + stringSuffix.toUtf8()));
60 QCOMPARE(extractedAlbum, QStringLiteral("Album1")); 62​ QCOMPARE(extractedAlbum, QString(QStringLiteral("Album1") + stringSuffix.toUtf8()));
smithjdUnsubmitted
Not Done

Is wrapping in a QString necessary?

smithjd: Is wrapping in a QString necessary?
astippichAuthorUnsubmitted
Not Done

It does not compile otherwise.

astippich: It does not compile otherwise.
61 QCOMPARE(extractedTrackNumber, 10u); 63​ QCOMPARE(extractedTrackNumber, 10u);
62 QCOMPARE(extractedYear, 1999u); 64​ QCOMPARE(extractedYear, 1999u);
63 QCOMPARE(extractedGenre, QStringLiteral("Genre1")); 65​ QCOMPARE(extractedGenre, QString(QStringLiteral("Genre1") + stringSuffix.toUtf8()));
64 QCOMPARE(extractedComment, QStringLiteral("Comment1")); 66​ QCOMPARE(extractedComment, QString(QStringLiteral("Comment1") + stringSuffix.toUtf8()));
svuorelaUnsubmitted
Not Done

I suggest checking that the last bytes of all these extracted things is the actual actual utf8 bytes, so that if someone compiles or saves this file in a broken encoding that the testing doesn't fail.

svuorela: I suggest checking that the last bytes of all these extracted things is the actual actual utf8…
brunsUnsubmitted
Not Done

?
The file/tags are written inside this test.

bruns: ? The file/tags are written inside this test.
svuorelaUnsubmitted
Not Done

yeah. given you write and read it, if somehow it gets encoded e.g. as iso-8859-15 rather than utf8, the euro sign would be encoded as 0xa4 rather than 0x20ac.

As you write and read it in the same sequence, there is a possibiliyt for this to pass when it shouldn't.

Unfortunately roundtripping the files with bad editors can make this happen. Especially on windows.

svuorela: yeah. given you write and read it, if somehow it gets encoded e.g. as iso-8859-15 rather than…
astippichAuthorUnsubmitted
Not Done

To be sure I understand correctly, using stringSuffix.toUTF8() is what you would like to see here?

astippich: To be sure I understand correctly, using stringSuffix.toUTF8() is what you would like to see…
svuorelaUnsubmitted
Not Done

Something along those lines, yes please, to compare with.

svuorela: Something along those lines, yes please, to compare with.
brunsUnsubmitted
Not Done

This should be covered by the read tests. The read tests use binary artifacts.

bruns: This should be covered by the read tests. The read tests use binary artifacts.
65 67​
66 QFile::remove(testFilePath(temporaryFileName)); 68​ QFile::remove(testFilePath(temporaryFileName));
67} 69​}
68 70​
69void TagLibWriterTest::testCommonData_data() 71​void TagLibWriterTest::testCommonData_data()
70{ 72​{
73​ QString unicodeTestStringSuffix = QStringLiteral("€");
brunsUnsubmitted
Done

If safeguarding against bad editors is really necessary, better do it here.
Alternatively, you can use C++11 unicode string literals, e.g.
QString(u"€")
If you save that one as e.g. latin1 and try to compile it, gcc reports:

error: converting to execution character set: Invalid or incomplete multibyte or wide character
     QString a{u"�"};
bruns: If safeguarding against bad editors is really necessary, better do it here. Alternatively, you…
astippichAuthorUnsubmitted
Not Done

Can this give failures on windows similar to D14122?

astippich: Can this give failures on windows similar to D14122?
brunsUnsubmitted
Done

Yes, probably.
Although, this gives also a pointer to a different approach:

// Add some unicode characters, use codepoints to avoid any issues with 
// source encoding: "€µ"
static const QChar data[4] = { 0x20ac, 0xb5 };
QString unicodeTestStringSuffix(data, 2);
bruns: Yes, probably. Although, this gives also a pointer to a different approach: ``` // Add some…
71 QTest::addColumn<QString>("fileType"); 74​ QTest::addColumn<QString>("fileType");
72 QTest::addColumn<QString>("mimeType"); 75​ QTest::addColumn<QString>("mimeType");
brunsUnsubmitted
Done

can you change this to data[2] ...
Facepalm myself ...

bruns: can you change this to `data[2]` ... Facepalm myself ...
astippichAuthorUnsubmitted
Not Done

I should have spotted this myself :)

astippich: I should have spotted this myself :)
76​ QTest::addColumn<QString>("stringSuffix");
svuorelaUnsubmitted
Not Done

Given stringSuffix is the same for all tests, is it needed to have as a test data thing ?

svuorela: Given stringSuffix is the same for all tests, is it needed to have as a test data thing ?
brunsUnsubmitted
Done

My idea here was to check each format twice, once with a simple latin1/ascii string (stringsuffix = "") and a second time with some unicode chars (e.g. "€", probably some more from other code blocks).

This allows to differentiate if only unicode tags are broken.

bruns: My idea here was to check each format twice, once with a simple latin1/ascii string…
astippichAuthorUnsubmitted
Not Done

That makes more sense of course, will update later to a unicode and a none unicode test

astippich: That makes more sense of course, will update later to a unicode and a none unicode test
73 77​
74 QTest::addRow("mp3") 78​ QTest::addRow("mp3")
75 << QStringLiteral("mp3") 79​ << QStringLiteral("mp3")
76 << QStringLiteral("audio/mpeg3") 80​ << QStringLiteral("audio/mpeg3")
81​ << QString()
82​ ;
83​
84​ QTest::addRow("mp3_unicode")
85​ << QStringLiteral("mp3")
86​ << QStringLiteral("audio/mpeg3")
87​ << unicodeTestStringSuffix
77 ; 88​ ;
78 89​
79 QTest::addRow("m4a") 90​ QTest::addRow("m4a")
80 << QStringLiteral("m4a") 91​ << QStringLiteral("m4a")
81 << QStringLiteral("audio/mp4") 92​ << QStringLiteral("audio/mp4")
93​ << QString()
94​ ;
95​
96​ QTest::addRow("m4a_unicode")
97​ << QStringLiteral("m4a")
98​ << QStringLiteral("audio/mp4")
99​ << unicodeTestStringSuffix
82 ; 100​ ;
83 101​
84 QTest::addRow("flac") 102​ QTest::addRow("flac")
85 << QStringLiteral("flac") 103​ << QStringLiteral("flac")
86 << QStringLiteral("audio/flac") 104​ << QStringLiteral("audio/flac")
105​ << QString()
106​ ;
107​
108​ QTest::addRow("flac_unicode")
109​ << QStringLiteral("flac")
110​ << QStringLiteral("audio/flac")
111​ << unicodeTestStringSuffix
87 ; 112​ ;
88 113​
89 QTest::addRow("opus") 114​ QTest::addRow("opus")
90 << QStringLiteral("opus") 115​ << QStringLiteral("opus")
91 << QStringLiteral("audio/opus") 116​ << QStringLiteral("audio/opus")
117​ << QString()
118​ ;
119​
120​ QTest::addRow("opus_unicode")
121​ << QStringLiteral("opus")
122​ << QStringLiteral("audio/opus")
123​ << unicodeTestStringSuffix
92 ; 124​ ;
93 125​
94 QTest::addRow("ogg") 126​ QTest::addRow("ogg")
95 << QStringLiteral("ogg") 127​ << QStringLiteral("ogg")
96 << QStringLiteral("audio/ogg") 128​ << QStringLiteral("audio/ogg")
129​ << QString()
130​ ;
131​
132​ QTest::addRow("ogg_unicode")
133​ << QStringLiteral("ogg")
134​ << QStringLiteral("audio/ogg")
135​ << unicodeTestStringSuffix
97 ; 136​ ;
98 137​
99 QTest::addRow("mpc") 138​ QTest::addRow("mpc")
100 << QStringLiteral("mpc") 139​ << QStringLiteral("mpc")
101 << QStringLiteral("audio/x-musepack") 140​ << QStringLiteral("audio/x-musepack")
141​ << QString()
142​ ;
143​
144​ QTest::addRow("mpc_unicode")
145​ << QStringLiteral("mpc")
brunsUnsubmitted
Done

^ unicodeTestStringSuffix ?

bruns: ^ unicodeTestStringSuffix ?
146​ << QStringLiteral("audio/x-musepack")
147​ << unicodeTestStringSuffix
102 ; 148​ ;
103} 149​}
104 150​
105QTEST_GUILESS_MAIN(TagLibWriterTest) 151​QTEST_GUILESS_MAIN(TagLibWriterTest)