This code creates a database and then table and delete one or more records.
- Maniphest Tasks
- T5091: [sok2017] Add record-deleting-related autotests
Run the tests
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.
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:
And similarly, this at the end:
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.
|183 ↗||(On Diff #10172)|
We're adding new line here so e.g. patch tools work better
|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.
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.
Very good, some minor comments.
<Coding style> Remove empty line
Please check and fix spacing after , and before ; in all the patch.
Please replace QVariant(10) with 10 in the entire patch to simplify the code. Casting to QVariant happens automatically.
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()?
Good. This helped to find a bug in our SQLite driver!
<Coding style> Empty line before
Thanks @staniek for merging this patch. Feels really good. Looking forward to contribute more :)
|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.
This Test Case Fails here. Trying to delete from a column that does not exist.