This code creates a database and then table and delete one or more records.
Details
- Reviewers
staniek - Maniphest Tasks
- T5091: [sok2017] Add record-deleting-related autotests
Run the tests
Diff Detail
- Repository
- R15 KDb
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
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!
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 |
- deleteRecords showing some different behaviour than deleting record in mysql.(in second last test case.)
Much better, thanks!
- General note - clarity: QVariant(***) is almost never needed, values are casted automatically to QVariant.
- 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 | ||
---|---|---|
123 | We're adding new line here so e.g. patch tools work better | |
autotests/DeleteItemsTest.h | ||
31 | For both names I propose delete -> deleteRecord for 100% clarity we're deleting records, not e.g. tables. | |
32 | Coding style: we're using camelCase for methods. |
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.
autotests/DeleteItemsTest.cpp | ||
---|---|---|
177 | Do we ask for fname = 56? Ok, that should work, a good test. |
- Merge branch 'master' of git://anongit.kde.org/kdb
- Added more tests and used existing code for accessing database and table.
Very good, some minor comments.
autotests/KDbTest.cpp | ||
---|---|---|
1122 ↗ | (On Diff #10268) | 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 ↗ | (On Diff #10268) | Good. This helped to find a bug in our SQLite driver! |
1166 ↗ | (On Diff #10268) | <Coding style> Remove empty line |
1174 ↗ | (On Diff #10268) | Please check and fix spacing after , and before ; in all the patch. |
1174 ↗ | (On Diff #10268) | Please replace QVariant(10) with 10 in the entire patch to simplify the code. Casting to QVariant happens automatically. |
1206 ↗ | (On Diff #10268) | <Coding style> Empty line before |
autotests/KDbTest.cpp | ||
---|---|---|
1161 ↗ | (On Diff #10268) | The value 56 is converted to "56" liberally so the call should succeed. |
autotests/KDbTest.cpp | ||
---|---|---|
1136 ↗ | (On Diff #10268) | The bug is closed now and the fix is in master branch. Please merge your branch and this very test will succeed. |
- removed spaces, removed QVarient
- Merge branch 'master' of git://anongit.kde.org/kdb
- All test cases passing
- Delete rows test: fix formatting and comments, supress warnings, add VERIFY2 comments
Updated the description to match the commit, I hope you don't mind. Changes are in master now :)
Thanks @staniek for merging this patch. Feels really good. Looking forward to contribute more :)
autotests/DeleteItemsTest.cpp | ||
---|---|---|
177 | 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. | |
177 | 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 ↗ | (On Diff #10268) | This Test Case Fails here. Trying to delete from a column that does not exist. |