Changeset View
Standalone View
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 | | |||
26 | void 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)); | ||
smithjd: Is wrapping in a QString necessary? | |||||
astippich: It does not compile otherwise. | |||||
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)); | ||
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… | |||||
bruns: ?
The file/tags are written inside this test. | |||||
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… | |||||
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… | |||||
svuorela: Something along those lines, yes please, to compare with.
| |||||
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 | | |||
69 | void TagLibWriterTest::testCommonData_data() | 71 | void TagLibWriterTest::testCommonData_data() | ||
70 | { | 72 | { | ||
73 | // Add some unicode characters, use codepoints to avoid any issues with | ||||
If safeguarding against bad editors is really necessary, better do it here. 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… | |||||
Can this give failures on windows similar to D14122? astippich: Can this give failures on windows similar to D14122? | |||||
Yes, probably. // 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[2] = { 0x20ac, 0xb5 }; | ||||
bruns: can you change this to `data[2]` ...
Facepalm 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"); | ||||
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 ? | |||||
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… | |||||
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 | << unicodeTestStringSuffix | ||||
bruns: ^ unicodeTestStringSuffix ? | |||||
102 | ; | 153 | ; | ||
103 | } | 154 | } | ||
104 | 155 | | |||
105 | QTEST_GUILESS_MAIN(TagLibWriterTest) | 156 | QTEST_GUILESS_MAIN(TagLibWriterTest) |
Is wrapping in a QString necessary?