team-port setting
ClosedPublic

Authored by pranavgade on Tue, Dec 4, 9:52 AM.

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.Tue, Dec 4, 9:52 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptTue, Dec 4, 9:52 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
pranavgade requested review of this revision.Tue, Dec 4, 9:52 AM
pranavgade updated this revision to Diff 46830.Tue, Dec 4, 9:56 AM

add missing indents

jgrulich added inline comments.Wed, Dec 5, 7:52 AM
autotests/settings/teamportsettingtest.cpp
30

Those defines are already in NM 1.10

63

set it to TRUE, otherwise it will fail once you fix the test

93

This is weird, this way only link-watchers should be compared, the rest should be identical to other unit tests.

src/settings/setting.cpp
34

NM 1.10.0

src/settings/teamportsetting.cpp
27

NM 1.10.0

211

0

215

!= 255

219

0

pranavgade updated this revision to Diff 46896.Wed, Dec 5, 12:09 PM
pranavgade marked 8 inline comments as done.
jgrulich added inline comments.Wed, Dec 5, 3:41 PM
autotests/settings/teamportsettingtest.cpp
98

Still weird, why don't you put link-watchers to the same map as above. You can then skip comparison if the key is "link-watchers" as you saw in ipv6settingtest for example.

121

Here you compare whether the maps have identical keys, which is correct, but you also have to compare the values.

src/settings/teamportsetting.cpp
45

Default property initialization is missing.

227

This is wrong, default value is FALSE, which means you would skip this property if it's set.

pranavgade updated this revision to Diff 46905.Wed, Dec 5, 4:06 PM
pranavgade marked 4 inline comments as done.
jgrulich added inline comments.Thu, Dec 6, 7:49 AM
autotests/settings/teamportsettingtest.cpp
101

This is again wrong, you want to go through "link-watchers" map, not through the whole map.

118

I think you can use QCOMPARE() here, to make it fail in case they don't match.

pranavgade updated this revision to Diff 46940.Thu, Dec 6, 8:43 AM
pranavgade marked 2 inline comments as done.
This comment was removed by jgrulich.
jgrulich added inline comments.Thu, Dec 6, 9:34 AM
autotests/settings/teamportsettingtest.cpp
104

Why don't you skip this and just use NMVariantMapList list = map.value(QLatin1String(NM_SETTING_TEAM_PORT_LINK_WATCHERS).value<NMVariantMapList>()? same for the second one. You don't need to go through the map to get "link-watchers" property.

pranavgade updated this revision to Diff 46946.Thu, Dec 6, 10:24 AM
pranavgade marked an inline comment as done.
jgrulich accepted this revision.Thu, Dec 6, 10:56 AM

I lived in assumption that "link-watchers" is NMVariantMapMap and not NMVariantMapList, that's why I suggested those changes. I'm sorry for that. I'll fix it locally here, you were correct before. The rest looks good.

This revision is now accepted and ready to land.Thu, Dec 6, 10:56 AM
This revision was automatically updated to reflect the committed changes.