Add tests for KDb::deleteRecord APIs
ClosedPublic

Authored by dwivedi on Jan 14 2017, 12:42 AM.

Details

Summary

This code creates a database and then table and delete one or more records.

Test Plan

Run the tests

Diff Detail

Repository
R15 KDb
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
dwivedi updated this revision to Diff 10159.Jan 14 2017, 12:42 AM
dwivedi retitled this revision from to sample test code for deleting row.
dwivedi updated this object.
dwivedi edited the test plan for this revision. (Show Details)
Restricted Application added a project: KDb. · View Herald TranscriptJan 14 2017, 12:42 AM
staniek requested changes to this revision.Jan 14 2017, 12:51 AM
staniek edited edge metadata.

Thanks Nitish. Based on how we test: we're using Qt Test for testing, not standalone apps written like above. Effectively it's similar but there are QVERIFY calls and many other checks. Please study http://doc.qt.io/qt-5/qtest-overview.html and especially see how our KDb tests are structured and implemented.

Thanks again!

This revision now requires changes to proceed.Jan 14 2017, 12:51 AM

See ConnectionTest for example use and that it uses QTEST_GUILESS_MAIN instead of main().

autotests/ItemDelete.cpp
134 ↗(On Diff #10159)

Q_VERIFY here

dwivedi updated this revision to Diff 10161.Jan 14 2017, 7:48 AM
dwivedi edited edge metadata.
dwivedi marked an inline comment as done.
  • revised and added QVERIFY calls for testing
dwivedi updated this revision to Diff 10167.Jan 14 2017, 2:31 PM
dwivedi edited edge metadata.
  • revisions in earlier tests
dwivedi updated this revision to Diff 10172.Jan 15 2017, 7:06 AM
  • deleteRecords showing some different behaviour than deleting record in mysql.(in second last test case.)
staniek added a comment.EditedJan 16 2017, 3:22 PM

Much better, thanks!

  1. General note - clarity: QVariant(***) is almost never needed, values are casted automatically to QVariant.
  1. General remark:

We have two independent tests now:

  • void deleteNonExistingrowTest();
  • void deleteWithTwoConstraintsTest();

How about this: instead of creating database by hand call this at the beginning of each of these tests:

QVERIFY(utils.testCreateDbWithTables("KDbTest"));

And similarly, this at the end:

QVERIFY(utils.testDisconnectAndDropDb());

When we add more database types, your tests would also use them fairy automatically.

Then we won't need to duplicate code and use the external "../tests/features/tables_test_p.h" many times.

autotests/DeleteItemsTest.cpp
183 ↗(On Diff #10172)

We're adding new line here so e.g. patch tools work better

autotests/DeleteItemsTest.h
30 ↗(On Diff #10172)

For both names I propose delete -> deleteRecord for 100% clarity we're deleting records, not e.g. tables.

31 ↗(On Diff #10172)

Coding style: we're using camelCase for methods.

https://community.kde.org/Policies/Kdelibs_Coding_Style

staniek added inline comments.Jan 16 2017, 3:26 PM
autotests/DeleteItemsTest.h
25 ↗(On Diff #10172)

I propose to move these methods to KDbTest after testTemporaryTableName(). It's a still a test KDbTest namespace after all.

30 ↗(On Diff #10172)

or deleteNonExistingRecordTest()

and deleteRecordWithTwoConstraintsTest()

General: I also think QLatin1String() is in most cases not needed in tests - we're using it only in KDb itself, and that's for performance. And when we want to make sure QVariant(QLatin1String(()) is of QString type and not QByteArray. Let's remove everywhere here for more compact code.

staniek added inline comments.Jan 16 2017, 4:30 PM
autotests/DeleteItemsTest.cpp
176 ↗(On Diff #10172)

Do we ask for fname = 56? Ok, that should work, a good test.

dwivedi updated this revision to Diff 10252.Jan 16 2017, 6:34 PM
  • Merge branch 'master' of git://anongit.kde.org/kdb
  • Added more tests and used existing code for accessing database and table.
dwivedi updated this revision to Diff 10254.Jan 16 2017, 7:42 PM
  • code moved to KDbtest
dwivedi updated this revision to Diff 10255.Jan 16 2017, 7:47 PM
  • test case
dwivedi updated this revision to Diff 10268.Jan 17 2017, 2:22 AM
  • Test Cases added
staniek requested changes to this revision.Feb 5 2017, 1:50 PM
staniek edited edge metadata.

Very good, some minor comments.

autotests/KDbTest.cpp
1122

I am getting a compile warning here:

kdb/autotests/KDbTest.cpp:1122: warning: passing NULL to non-pointer argument 4 of ‘bool KDb::deleteRecords(KDbConnection*, const QString&, const QString&, int)’ [-Wconversion-null]
     QVERIFY(KDb::deleteRecords(utils.connection.data(), "persons", "name", NULL));
                                                                                                    ^

As you can see the compiler converted NULL to the int keyval. Did you mean a QString()?

1136

Good. This helped to find a bug in our SQLite driver!
See https://bugs.kde.org/show_bug.cgi?id=376052

1166

<Coding style> Remove empty line

1174

Please check and fix spacing after , and before ; in all the patch.

1174

Please replace QVariant(10) with 10 in the entire patch to simplify the code. Casting to QVariant happens automatically.

1206

<Coding style> Empty line before

This revision now requires changes to proceed.Feb 5 2017, 1:50 PM
staniek added inline comments.Feb 5 2017, 11:26 PM
autotests/KDbTest.cpp
1161

The value 56 is converted to "56" liberally so the call should succeed.

staniek added inline comments.Feb 5 2017, 11:46 PM
autotests/KDbTest.cpp
1136

The bug is closed now and the fix is in master branch. Please merge your branch and this very test will succeed.

dwivedi updated this revision to Diff 11098.Feb 9 2017, 5:23 AM
dwivedi edited edge metadata.
  • removed spaces, removed QVarient
  • Merge branch 'master' of git://anongit.kde.org/kdb
  • All test cases passing
staniek updated this revision to Diff 11208.Feb 11 2017, 11:15 AM
staniek edited edge metadata.
  • Delete rows test: fix formatting and comments, supress warnings, add VERIFY2 comments
staniek accepted this revision.Feb 11 2017, 11:27 AM
staniek edited edge metadata.

Good job Nithish! I like it very much. It helped to improve KDb!

This revision is now accepted and ready to land.Feb 11 2017, 11:27 AM
staniek closed this revision.Feb 11 2017, 11:32 AM
staniek updated this object.
staniek edited the test plan for this revision. (Show Details)
staniek edited edge metadata.
staniek retitled this revision from sample test code for deleting row to Add tests for KDb::deleteRecord APIs .

Updated the description to match the commit, I hope you don't mind. Changes are in master now :)

dwivedi marked 6 inline comments as done.Feb 11 2017, 12:32 PM

Thanks @staniek for merging this patch. Feels really good. Looking forward to contribute more :)

autotests/DeleteItemsTest.cpp
176 ↗(On Diff #10172)

In this test, the deleteRecords() function is failing. What i did is passed an Integer value when it is expecting a String. The query in the comments above shows what kind of error is expected. But instead of that , it does executes the query successfully leading the test to fail.

176 ↗(On Diff #10172)

No, fname = 56 does not exist. Its just a random number that i choose. Only motive behind this test was to check how it handles a type of data which it does not expect.

autotests/KDbTest.cpp
1136

This Test Case Fails here. Trying to delete from a column that does not exist.