D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:4d48330829d9: Convert string formatting tests to be data 
driven (authored by bruns).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20032?vs=55073=55322

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

AFFECTED FILES
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added a comment.


  In D20032#442635 , @apol wrote:
  
  > Whatever.
  >  @bruns Have you considered becoming the module maintainer? :)
  
  
  Thats fine for me. Where do I have to deliver my soul? ;-)

REPOSITORY
  R286 KFileMetaData

BRANCH
  submit

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

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

D20032: Convert string formatting tests to be data driven

[Aleix Pol Gonzalez]

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


  Whatever.
  @bruns Have you considered becoming the module maintainer? :)

REPOSITORY
  R286 KFileMetaData

BRANCH
  submit

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> apol wrote in propertyinfotest.cpp:71
> Not really, but if @mgallien, who is the maintainer, is fine with it, I'm 
> fine too.
> After all it's still better than what we used to have.

@mgallien has not done any coding are reviews on kfilemetadata for almost a 
year ...

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns updated this revision to Diff 55073.
bruns added a comment.


  rebase

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20032?vs=54741=55073

BRANCH
  submit

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

AFFECTED FILES
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h

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

D20032: Convert string formatting tests to be data driven

[Aleix Pol Gonzalez]

apol added a subscriber: mgallien.
apol added inline comments.

INLINE COMMENTS

> bruns wrote in propertyinfotest.cpp:71
> Still not convinced?

Not really, but if @mgallien, who is the maintainer, is fine with it, I'm fine 
too.
After all it's still better than what we used to have.

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added a comment.


  @apol - do you have a proposal how to avoid the "weird struct" and still keep 
it readable? Preferably, no repetition of the Property, and no extra explicit 
constructors.

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> bruns wrote in propertyinfotest.cpp:71
> First 2 rows expanded, just for you:
> 
>   QTest::addRow("%s", 
> PropertyInfo(DiscNumber).displayName().toUtf8().constData()) << 
> PropertyInfo(Property::DiscNumber) << true << QVariant(2018) << 
> QStringLiteral("2018");
>   QTest::addRow("%s", PropertyInfo(Title).displayName().toUtf8().constData()) 
> << PropertyInfo(Property::Title) << true << QVariant(QStringLiteral("Title")) 
> << QStringLiteral("Title");

Still not convinced?

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> bruns wrote in propertyinfotest.cpp:71
> This saves adding the `QVariant(...)` around each value, and avoids the 
> repeated formatting of the row name/data index. The property enum is used 
> twice in each addRow.

And instead it makes you create a weird local struct and loop to feed to QTest. 
I don't find it very convincing.

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> apol wrote in propertyinfotest.cpp:71
> And instead it makes you create a weird local struct and loop to feed to 
> QTest. I don't find it very convincing.

First 2 rows expanded, just for you:

  QTest::addRow("%s", 
PropertyInfo(DiscNumber).displayName().toUtf8().constData()) << 
PropertyInfo(Property::DiscNumber) << true << QVariant(2018) << 
QStringLiteral("2018");
  QTest::addRow("%s", PropertyInfo(Title).displayName().toUtf8().constData()) 
<< PropertyInfo(Property::Title) << true << QVariant(QStringLiteral("Title")) 
<< QStringLiteral("Title");

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> apol wrote in propertyinfotest.cpp:71
> Is the struct really necessary?
> You can be about as short by calling QTest::addRow directly for each line.

This saves adding the `QVariant(...)` around each value, and avoids the 
repeated formatting of the row name/data index. The property enum is used twice 
in each addRow.

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Aleix Pol Gonzalez]

apol added a comment.


  +1 to _data splitting, makes a lot of sense.

INLINE COMMENTS

> propertyinfotest.cpp:71
>  
> -PropertyInfo bitRate(Property::BitRate);
> -QCOMPARE(bitRate.formatAsDisplayString(QVariant(128000)), 
> QStringLiteral("128 kbit/s"));
> -QCOMPARE(bitRate.formatAsDisplayString(QVariant(135)), 
> QString(QLocale().toString(1.35) + QStringLiteral(" Mbit/s")));
> -QCOMPARE(bitRate.formatAsDisplayString(QVariant(1470)), 
> QString(QLocale().toString(14.7) + QStringLiteral(" Mbit/s")));
> -
> -PropertyInfo orientation(Property::ImageOrientation);
> -QCOMPARE(orientation.formatAsDisplayString(QVariant(5)), 
> QStringLiteral("Transposed"));
> -
> -PropertyInfo flash(Property::PhotoFlash);
> -QCOMPARE(flash.formatAsDisplayString(QVariant(0x00)), QStringLiteral("No 
> flash"));
> -QCOMPARE(flash.formatAsDisplayString(QVariant(0x50)), 
> QStringLiteral("No, red-eye reduction"));
> -
> -PropertyInfo altitude(Property::PhotoGpsAltitude);
> -QCOMPARE(altitude.formatAsDisplayString(QVariant(1.1)), 
> QString(QLocale().toString(1.1) + QStringLiteral(" m")));
> -
> -PropertyInfo latitude(Property::PhotoGpsLatitude);
> -// make tests on windows happy: QChar(0x00B0) = "°"
> -QCOMPARE(latitude.formatAsDisplayString(QVariant(25)), 
> QString(QLocale().toString(25) + QChar(0x00B0)));
> -
> -PropertyInfo longitude(Property::PhotoGpsLongitude);
> -QCOMPARE(longitude.formatAsDisplayString(QVariant(13.5)), 
> QString(QLocale().toString(13.5) + QChar(0x00B0)));
> -
> -PropertyInfo framerate(Property::FrameRate);
> -QCOMPARE(framerate.formatAsDisplayString(QVariant(23)), 
> QStringLiteral("23 fps"));
> -QCOMPARE(framerate.formatAsDisplayString(QVariant(23.976)), 
> QStringLiteral("23.98 fps"));
> -
> -PropertyInfo aspectRatio(Property::AspectRatio);
> -QCOMPARE(aspectRatio.formatAsDisplayString(QVariant(1.8)), 
> QStringLiteral("1.78:1"));
> +struct {
> +KFileMetaData::Property::Property property;

Is the struct really necessary?
You can be about as short by calling QTest::addRow directly for each line.

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added a dependent revision: D20033: Default string formatting test to C 
locale, add localized run.

REPOSITORY
  R286 KFileMetaData

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

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

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

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

REVISION SUMMARY
  Less boilerplate code in the actual test data.
  
  Depends on D20031 

TEST PLAN
  LANG=C ctest

REPOSITORY
  R286 KFileMetaData

BRANCH
  submit

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

AFFECTED FILES
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h

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