Batch remove entries #5659
Batch remove entries #5659
Conversation
| @@ -164,18 +164,46 @@ void testUpdateEqualEntry(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMS | |||
|
|
|||
| @ParameterizedTest | |||
| @MethodSource("org.jabref.logic.shared.TestManager#getTestingDatabaseSystems") | |||
| void testRemoveEntry(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws SQLException { | |||
There was a problem hiding this comment.
Can be keep the single test? -> Meaning the additional test for two entries is OK, but we should also keep the simple (and probably always succeeding test). - Reason: The for loop above is not entered for one entry and thus this test case is special.
There was a problem hiding this comment.
I could, but it would still have to be dbmsProcessor.removeEntries(Collections.singletonList(entryToRemove)) because I got rid of DBMSProcessor.removeEntry in the singular. Is it still worth it?
There was a problem hiding this comment.
Sure. The tests should be independent of the implemention somehow. Tests should cover the general case, but also edge cases, null list, 0 element list, list with 1 elements, list with integer.max elements (which is impossible IMHO), but the other cases should be easy to do?
There was a problem hiding this comment.
Okay, in that case I will use Collections.singletonList. Should I add a test for each of those edge cases as well?
There was a problem hiding this comment.
Sure, please go ahead. In case tests are easy to write, they should be written. - The thing is: The implementation could change in 1 year (who knows) and the test will still cover that case.
Please also add a test for
dbmsProcessor.removeEntries(Collections.emptyList())dbmsProcessor.removeEntries(null)(should throw a NPE)
There was a problem hiding this comment.
Throw on empty list? If so, throw what? I can't imagine why someone would want to call removeEntries on purpose on an empty list.
|
Minor thing: checkstyle should be fixed 😇 |
| .append(" = ?"); | ||
| .append(" IN ("); | ||
| for (int i = 0; i < bibEntries.size() - 1; i++) { | ||
| query.append("?, "); |
There was a problem hiding this comment.
I think you can use "?, ".repeat(bibEntries.size())` here
There was a problem hiding this comment.
Made the change, will push soon.
|
I merged to move ahead. Think, the software engineering discussion can be done later :)
https://sqa.stackexchange.com/a/9009/28592 Thus, I argue for improper usage - who knows, what future code will bring. |
|
Thanks! Is there any way I can fix this in my following PR? |
I changed the SQL in
DBMSProcessorso that multiple entries are deleted with one query instead of using a query for each entry. This was the original rationale for the refactoring I did in October.