Differential D15714 Diff 42525 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, "Title1"); 39​
39 data.add(Property::Artist, "Artist1"); 40​ data.add(Property::Title, QString(QStringLiteral("Title1") + stringSuffix));
40 data.add(Property::Album, "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, "Genre1"); 45​ data.add(Property::Genre, QString(QStringLiteral("Genre1") + stringSuffix));
44 data.add(Property::Comment, "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));
59 QCOMPARE(extractedArtist, QStringLiteral("Artist1")); 61​ QCOMPARE(extractedArtist, QString(QStringLiteral("Artist1") + stringSuffix));
60 QCOMPARE(extractedAlbum, QStringLiteral("Album1")); 62​ QCOMPARE(extractedAlbum, QString(QStringLiteral("Album1") + stringSuffix));
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.
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…
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));
64 QCOMPARE(extractedComment, QStringLiteral("Comment1")); 66​ QCOMPARE(extractedComment, QString(QStringLiteral("Comment1") + stringSuffix));
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​ // Add some unicode characters, use codepoints to avoid any issues with
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…
74​ // source encoding: "€µ"
75​ static const QChar data[4] = { 0x20ac, 0xb5 };
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​ QString unicodeTestStringSuffix(data, 2);
77​
71 QTest::addColumn<QString>("fileType"); 78​ QTest::addColumn<QString>("fileType");
72 QTest::addColumn<QString>("mimeType"); 79​ QTest::addColumn<QString>("mimeType");
80​ 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
81​
73 82​
74 QTest::addRow("flac") 83​ QTest::addRow("flac")
75 << QStringLiteral("flac") 84​ << QStringLiteral("flac")
76 << QStringLiteral("audio/flac") 85​ << QStringLiteral("audio/flac")
86​ << QString()
87​ ;
88​
89​ QTest::addRow("flac_unicode")
90​ << QStringLiteral("flac")
91​ << QStringLiteral("audio/flac")
92​ << unicodeTestStringSuffix
77 ; 93​ ;
78 94​
79 QTest::addRow("m4a") 95​ QTest::addRow("m4a")
80 << QStringLiteral("m4a") 96​ << QStringLiteral("m4a")
81 << QStringLiteral("audio/mp4") 97​ << QStringLiteral("audio/mp4")
98​ << QString()
99​ ;
100​
101​ QTest::addRow("m4a_unicode")
102​ << QStringLiteral("m4a")
103​ << QStringLiteral("audio/mp4")
104​ << unicodeTestStringSuffix
82 ; 105​ ;
83 106​
84 QTest::addRow("mp3") 107​ QTest::addRow("mp3")
85 << QStringLiteral("mp3") 108​ << QStringLiteral("mp3")
86 << QStringLiteral("audio/mpeg3") 109​ << QStringLiteral("audio/mpeg3")
110​ << QString()
111​ ;
112​
113​ QTest::addRow("mp3_unicode")
114​ << QStringLiteral("mp3")
115​ << QStringLiteral("audio/mpeg3")
116​ << unicodeTestStringSuffix
87 ; 117​ ;
88 118​
89 QTest::addRow("mpc") 119​ QTest::addRow("mpc")
90 << QStringLiteral("mpc") 120​ << QStringLiteral("mpc")
91 << QStringLiteral("audio/x-musepack") 121​ << QStringLiteral("audio/x-musepack")
122​ << QString()
123​ ;
124​
125​ QTest::addRow("mpc_unicode")
126​ << QStringLiteral("mpc")
127​ << QStringLiteral("audio/x-musepack")
128​ << unicodeTestStringSuffix
92 ; 129​ ;
93 130​
94 QTest::addRow("ogg") 131​ QTest::addRow("ogg")
95 << QStringLiteral("ogg") 132​ << QStringLiteral("ogg")
96 << QStringLiteral("audio/ogg") 133​ << QStringLiteral("audio/ogg")
134​ << QString()
135​ ;
136​
137​ QTest::addRow("ogg_unicode")
138​ << QStringLiteral("ogg")
139​ << QStringLiteral("audio/ogg")
140​ << unicodeTestStringSuffix
97 ; 141​ ;
98 142​
99 QTest::addRow("opus") 143​ QTest::addRow("opus")
100 << QStringLiteral("opus") 144​ << QStringLiteral("opus")
101 << QStringLiteral("audio/opus") 145​ << QStringLiteral("audio/opus")
146​ << QString()
147​ ;
148​
149​ QTest::addRow("opus_unicode")
150​ << QStringLiteral("opus")
151​ << QStringLiteral("audio/opus")
152​ << QStringLiteral("€")
brunsUnsubmitted
Done

^ unicodeTestStringSuffix ?

bruns: ^ unicodeTestStringSuffix ?
102 ; 153​ ;
103} 154​}
104 155​
105QTEST_GUILESS_MAIN(TagLibWriterTest) 156​QTEST_GUILESS_MAIN(TagLibWriterTest)