D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
astippich marked an inline comment as done.
Closed by commit R286:196b1289152c: add a string suffix to test data and use 
for unicode testing of taglibwriter (authored by astippich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D15714?vs=42541=43327#toc

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15714?vs=42541=43327

REVISION DETAIL
  https://phabricator.kde.org/D15714

AFFECTED FILES
  autotests/taglibwritertest.cpp

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich marked an inline comment as done.
astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwritertest.cpp:75
> can you change this to `data[2]` ...
> Facepalm myself ...

I should have spotted this myself :)

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_write_unicode

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_write_unicode

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns accepted this revision.
bruns added a comment.
This revision is now accepted and ready to land.


  Otherwise good to go ...

INLINE COMMENTS

> taglibwritertest.cpp:75
> +// source encoding: "€µ"
> +static const QChar data[4] = { 0x20ac, 0xb5 };
> +QString unicodeTestStringSuffix(data, 2);

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_write_unicode

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich marked 5 inline comments as done.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich updated this revision to Diff 42541.
astippich added a comment.


  - fix after rebase

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15714?vs=42525=42541

BRANCH
  taglib_write_unicode

REVISION DETAIL
  https://phabricator.kde.org/D15714

AFFECTED FILES
  autotests/taglibwritertest.cpp

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> taglibwritertest.cpp:152
> +<< QStringLiteral("audio/opus")
> +<< QStringLiteral("€")
>  ;

^ unicodeTestStringSuffix ?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns added a comment.


  +1
  @svuorela ?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich updated this revision to Diff 42525.
astippich added a comment.


  - use suggested test pattern

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15714?vs=42270=42525

BRANCH
  taglib_write_unicode

REVISION DETAIL
  https://phabricator.kde.org/D15714

AFFECTED FILES
  autotests/taglibwritertest.cpp

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> astippich wrote in taglibwritertest.cpp:73
> Can this give failures on windows similar to D14122 
> ?

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);

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwritertest.cpp:73
> 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"�"};

Can this give failures on windows similar to D14122 
?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> taglibwritertest.cpp:60
>  
> -QCOMPARE(extractedTitle, QStringLiteral("Title1"));
> -QCOMPARE(extractedArtist, QStringLiteral("Artist1"));
> -QCOMPARE(extractedAlbum, QStringLiteral("Album1"));
> +QCOMPARE(extractedTitle, QString(QStringLiteral("Title1") + 
> stringSuffix.toUtf8()));
> +QCOMPARE(extractedArtist, QString(QStringLiteral("Artist1") + 
> stringSuffix.toUtf8()));

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.

> taglibwritertest.cpp:73
>  {
> +QString unicodeTestStringSuffix = QStringLiteral("€");
>  QTest::addColumn("fileType");

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"�"};

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> smithjd wrote in taglibwritertest.cpp:60
> Is wrapping in a QString necessary?

It does not compile otherwise.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich updated this revision to Diff 42270.
astippich added a comment.


  - actually use temporary string variable for unicode

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15714?vs=42264=42270

BRANCH
  taglib_write_unicode

REVISION DETAIL
  https://phabricator.kde.org/D15714

AFFECTED FILES
  autotests/taglibwritertest.cpp

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich updated this revision to Diff 42264.
astippich added a comment.


  - implement feedback

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15714?vs=42197=42264

BRANCH
  taglib_write_unicode

REVISION DETAIL
  https://phabricator.kde.org/D15714

AFFECTED FILES
  autotests/taglibwritertest.cpp

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[James Smith]

smithjd added inline comments.

INLINE COMMENTS

> taglibwritertest.cpp:60
>  
> -QCOMPARE(extractedTitle, QStringLiteral("Title1"));
> -QCOMPARE(extractedArtist, QStringLiteral("Artist1"));
> -QCOMPARE(extractedAlbum, QStringLiteral("Album1"));
> +QCOMPARE(extractedTitle, QString(QStringLiteral("Title1") + 
> stringSuffix));
> +QCOMPARE(extractedArtist, QString(QStringLiteral("Artist1") + 
> stringSuffix));

Is wrapping in a QString necessary?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> svuorela wrote in taglibwritertest.cpp:66
> 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.

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

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> astippich wrote in taglibwritertest.cpp:66
> To be sure I understand correctly, using stringSuffix.toUTF8() is what you 
> would like to see here?

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

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> svuorela wrote in taglibwritertest.cpp:66
> 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.

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

> bruns wrote in taglibwritertest.cpp:75
> 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.

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

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwritertest.cpp:66
> ?
> 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.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> svuorela wrote in taglibwritertest.cpp:66
> 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.

?
The file/tags are written inside this test.

> svuorela wrote in taglibwritertest.cpp:75
> 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.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> taglibwritertest.cpp:66
> +QCOMPARE(extractedGenre, QString(QStringLiteral("Genre1") + 
> stringSuffix));
> +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.

> taglibwritertest.cpp:75
>  QTest::addColumn("mimeType");
> +QTest::addColumn("stringSuffix");
>  

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

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  append a unicode character to all test string such that unicode characters 
can be tests

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_write_unicode

REVISION DETAIL
  https://phabricator.kde.org/D15714

AFFECTED FILES
  autotests/taglibwritertest.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich added a dependency: D15704: increase test coverage of taglibwriter.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams