Convert string formatting tests to be data driven
ClosedPublic

Authored by bruns on Mar 25 2019, 12:41 AM.

Details

Summary

Less boilerplate code in the actual test data.

Depends on D20031

Test Plan

LANG=C ctest

Diff Detail

Repository
R286 KFileMetaData
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bruns created this revision.Mar 25 2019, 12:41 AM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptMar 25 2019, 12:41 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bruns requested review of this revision.Mar 25 2019, 12:41 AM
apol added a subscriber: apol.Mar 25 2019, 11:24 AM

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

autotests/propertyinfotest.cpp
71–102

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

bruns added inline comments.Mar 25 2019, 12:27 PM
autotests/propertyinfotest.cpp
71–102

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.

apol added inline comments.Mar 25 2019, 1:58 PM
autotests/propertyinfotest.cpp
71–102

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

bruns added inline comments.Mar 25 2019, 2:06 PM
autotests/propertyinfotest.cpp
71–102

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");
bruns added inline comments.Mar 27 2019, 6:41 PM
autotests/propertyinfotest.cpp
71–102

Still not convinced?

bruns added a comment.Mar 29 2019, 5:17 PM

@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.

apol added a subscriber: mgallien.Mar 29 2019, 6:05 PM
apol added inline comments.
autotests/propertyinfotest.cpp
71–102

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.

bruns added inline comments.Mar 31 2019, 3:23 PM
autotests/propertyinfotest.cpp
71–102

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

apol accepted this revision.Apr 3 2019, 12:02 AM

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

This revision is now accepted and ready to land.Apr 3 2019, 12:02 AM
bruns added a comment.Apr 3 2019, 12:04 AM
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? ;-)

This revision was automatically updated to reflect the committed changes.