match and tc setting
ClosedPublic

Authored by pranavgade on Sun, Dec 2, 4:28 PM.

Diff Detail

Repository
R282 NetworkManagerQt
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
pranavgade created this revision.Sun, Dec 2, 4:28 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSun, Dec 2, 4:28 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
pranavgade requested review of this revision.Sun, Dec 2, 4:28 PM

Why did you add my test for tun setting?

jgrulich added inline comments.Sun, Dec 2, 5:48 PM
autotests/settings/matchsettingtest.cpp
44

This define won't exist for NM < 1.12.0

autotests/settings/tcsettingtest.cpp
59

TC defines do not exist before NM 1.10.0

70

I think that you cannot compare NMVariantMaps this way, that's why probably the test for IPv4 and IPv6 didn't fail when we swapped values for route-data and address-data. You will need to compare values inside the maps separately.

src/settings/matchsetting.h
28

I think you don't need to include <QString>

30

Move those defines out of the header file.

src/settings/tcsetting.cpp
116

const QVariantMap &qdisc

119

Indent.

123

const QVariantMap &tfilter

src/settings/tcsetting.h
31

TC defines are present since NM 1.10.0. Also move them out of the header file.

src/settings/tcsetting_p.h
26

This include shouldn't be needed.

pranavgade updated this revision to Diff 46761.Mon, Dec 3, 2:19 AM
pranavgade marked 9 inline comments as done.
pranavgade added inline comments.
autotests/settings/tcsettingtest.cpp
70

So..how should I do it?

jgrulich added inline comments.Mon, Dec 3, 7:18 AM
autotests/settings/tcsettingtest.cpp
70

See below, it's ugly, but problem is that give it's a QList, the maps can be in different order so you first have to identify you compare correct QVariantMaps. I do that by comparing keys.

75

Something like:

NMVariantMapList list = it.value();
NMVariantMapList list1 = map1.value(it.key());
QCOMPARE(list.count(), list1.count());

int comparedMaps = 0;
NMVariantMapList::const_iterator listIt = list.constBegin();
while (listIt != list.constEnd() {
    NMVariantMapList::const_iterator list1It = list1.constBegin();
    while (list1it != list1.constEnd()) {
         QVariantMap listMap = listIt.value();
         QVariantMap1 listMap1 = list1It.value();
         // Test if keys do match, because the list can be in different order
         QStringList listMapKeys = listMap.keys();
         QStringList listMapKeys1 = listMap1.keys();
         listMapKeys.sort();
         listMapKeys1.sort();
         if (listMapKeys.join(QChar(' ')) ==  listMapKeys1.join(QChar(' '))) {
               // Here the maps should have same keys so compare QVariantMaps as we do now
               .........
               ++comparedMaps;
         }
         ++list1it;  
     }
     ++listIt;
}

// Test if we compared all maps, if not, then probably they didn't match
QCOMPARE(comparedMaps, list.count();
jgrulich added inline comments.Mon, Dec 3, 12:28 PM
autotests/settings/matchsettingtest.cpp
30

NM 1.14.0, which is not released yet.

autotests/settings/tcsettingtest.cpp
75

It should be:

// Will fail if set some default values, because they are skipped in toMap() method
QVariantMap::const_iterator it = map.constBegin();
while (it != map.constEnd()) {
    NMVariantMapList list = it.value().value<NMVariantMapList>();
    NMVariantMapList list1 = map1.value(it.key()).value<NMVariantMapList>();
    QCOMPARE(list.count(), list1.count());

    int comparedMaps = 0;
    NMVariantMapList::const_iterator listIt = list.constBegin();
    while (listIt != list.constEnd()) {
        NMVariantMapList::const_iterator list1It = list1.constBegin();
        while (list1It != list1.constEnd()) {
            QVariantMap listMap = *listIt;
            QVariantMap listMap1 = *list1It;
            // Test if keys do match, because the list can be in different order
            QStringList listMapKeys = listMap.keys();
            QStringList listMapKeys1 = listMap1.keys();
            listMapKeys.sort();
            listMapKeys1.sort();
            if (listMapKeys.join(QChar(' ')) ==  listMapKeys1.join(QChar(' '))) {
                // Here the maps should have same keys so compare QVariantMaps as we do now
                .........
                ++comparedMaps;
            }
            ++list1It;
        }
        ++listIt;
    }
    ++it;
}

You just need to add missing comparison of variant maps, which should be trivial.

src/settings/matchsetting.cpp
27

This is actually in master only, which means upcoming NM 1.14.0.

src/settings/setting.cpp
33

Same here, should be NM 1.14.0.

pranavgade updated this revision to Diff 46785.Mon, Dec 3, 1:30 PM
pranavgade marked 5 inline comments as done.
pranavgade updated this revision to Diff 46786.Mon, Dec 3, 1:33 PM
pranavgade marked 3 inline comments as done.
jgrulich added inline comments.Mon, Dec 3, 1:39 PM
autotests/settings/tcsettingtest.cpp
93

You still don't compare the values.

src/settings/setting.cpp
33

NM 1.14.0 is needed for NM_SETTING_MATCH_SETTING_NAME

src/settings/tcsetting.cpp
119

Still applies.

123

Still applies.

src/settings/tcsetting.h
28

No need to include QString and QStringList

pranavgade updated this revision to Diff 46789.Mon, Dec 3, 2:52 PM
pranavgade marked 4 inline comments as done.

And you submitted only changes on top of your changes.

autotests/settings/tcsettingtest.cpp
93

I liked your original solution better, I mean the way you compared if you are going to compare correct maps. Anyway, this still doesn't compare the maps properly, you have to go value by value.

pranavgade updated this revision to Diff 46792.Mon, Dec 3, 3:39 PM
pranavgade marked an inline comment as done.

Can you please rebase your changes on top of the current master? I did some changes there and the patch is not applicable.

pranavgade updated this revision to Diff 46824.Tue, Dec 4, 8:10 AM

The tcsettingtest is failing.

The tcsettingtest is failing.

Why, exactly?

The tcsettingtest is failing.

Why, exactly?

Because it's broken in numerous ways :).

void TcSettingTest::testSetting()
{
    QFETCH(NMVariantMapList, tfilters);
    QFETCH(NMVariantMapList, qdiscs);

    QVariantMap map;

    map.insert(QLatin1String(NM_SETTING_TC_CONFIG_TFILTERS), QVariant::fromValue(tfilters));
    map.insert(QLatin1String(NM_SETTING_TC_CONFIG_QDISCS), QVariant::fromValue(qdiscs));

    NetworkManager::TcSetting setting;
    setting.fromMap(map);

    QVariantMap map1 = setting.toMap();

    QVariantMap::const_iterator it = map.constBegin();
    while (it != map.constEnd()) {
        NMVariantMapList list = it.value().value<NMVariantMapList>();
        NMVariantMapList list1 = map1.value(it.key()).value<NMVariantMapList>();

        QCOMPARE(list.count(), list1.count());

        int comparedMaps = 0;
        for (int i = 0; i < list.size(); ++i) {
            QVariantMap varMap = list.at(i);
            for (int j = 0; j < list1.size(); ++j) {
                QVariantMap varMap1 = list1.at(i);
                QVariantMap::const_iterator ite = varMap.constBegin();
                int comparedvals = 0;
                while (ite != varMap.constEnd()) {
                    QVariantMap::const_iterator val1 = varMap1.constFind(ite.key());
                    if (val1 != varMap1.constEnd()) {
                        if (varMap.value(ite.key()) == val1.value()) {
                            ++comparedvals;
                        }
                    }
                    ++ite;
                }
                if (comparedvals == varMap.size()) {
                    comparedMaps++;
                }
            }
        }
        ++it;
        QCOMPARE(comparedMaps, list.count());
    }
}

It should be like this.

pranavgade updated this revision to Diff 46825.Tue, Dec 4, 8:56 AM

It again doesn't apply, rebase it to current master.

pranavgade updated this revision to Diff 46826.Tue, Dec 4, 9:15 AM
jgrulich accepted this revision.Tue, Dec 4, 9:29 AM
This revision is now accepted and ready to land.Tue, Dec 4, 9:29 AM
This revision was automatically updated to reflect the committed changes.