pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.
- same notes for `simpleipv6test.cpp` as in `simpleipv4test.cpp`
INLINE COMMENTS
> CMakeLists.txt:11-13
> +add_executable(simpleipv6test ${simpleipv6test_SRCS})
> +add_test(simpleipv6 simpleipv6test)
> +ecm_mark_as_test(simpleipv6test)
just use `ecm_add_test` instead
> CMakeLists.txt:18-19
> + Qt5::Widgets
> + KF5::ConfigCore
> + KF5::CoreAddons)
> +add_library(simpleipv6test_SRCS)
are these two libraries actually used?
> CMakeLists.txt:26-28
> +add_executable(simpleipv4test ${simpleipv4test_SRCS})
> +add_test(simpleipv4 simpleipv4test)
> +ecm_mark_as_test(simpleipv4test)
ditto
> CMakeLists.txt:33-34
> + Qt5::Widgets
> + KF5::ConfigCore
> + KF5::CoreAddons)
> +add_library(simpleipv4test_SRCS)
ditto
> simpleipv4test.cpp:23
> +#include <QTest>
> +#include <QDir>
> +
unused
> simpleipv4test.cpp:43-45
> + vb = new SimpleIpV4AddressValidator(nullptr,
> SimpleIpV4AddressValidator::AddressStyle::Base);
> + vc = new SimpleIpV4AddressValidator(nullptr,
> SimpleIpV4AddressValidator::AddressStyle::WithCidr);
> + vp = new SimpleIpV4AddressValidator(nullptr,
> SimpleIpV4AddressValidator::AddressStyle::WithPort);
even if this is a test, these are leaked, "polluting" the results of tools like
valgrind; just make them non-pointers class variables
> simpleipv4test.cpp:48
> +
> +void SimpleIpv4Test::baseTest()
> +{
please use a data-driven approach for this file
> simpleipv4test.cpp:53
> + QString tstr("");
> + QVERIFY(vb->validate(tstr, pos) == QValidator::Intermediate);
> +
`QVERIFY(foo == bar)` -> `QCOMPARE(foo, bar)`, it applies to all the checks in
this file
REPOSITORY
R116 Plasma Network Management Applet
REVISION DETAIL
https://phabricator.kde.org/D15520
To: andersonbruce, jgrulich, pino
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
