[UnIndexedFileIteratorTest] Add tests
Needs ReviewPublic

Authored by poboiko on May 31 2019, 9:02 AM.

Details

Reviewers
bruns
Group Reviewers
Frameworks
Baloo
Summary

Add tests for UnIndexedFileIterator, which cover basic cases.

Right now it fails, see D21427: Always skip trailing slashes in FilderedDirIterator (one of the folders has trailing slash, while it should not)

Test Plan

Diff Detail

Repository
R293 Baloo
Branch
unindexed-renamed
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12363
Build 12381: arc lint + arc unit
poboiko created this revision.May 31 2019, 9:02 AM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptMay 31 2019, 9:02 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
poboiko requested review of this revision.May 31 2019, 9:02 AM
bruns requested changes to this revision.Jun 2 2019, 11:41 PM

The unit test lacks structure, please think about:

  • how you can split this into separate test cases (basic functionality, renaming, ...)
  • how to use a data driven approach for the test, i.e. splitting the basic test into one entry per file
autotests/unit/file/unindexedfileiteratortest.cpp
125–146

Bad variable name, these are files and dirs.

129

bad name, use something like excluded.

165

you don't actually have to determine the mimetype here, just use "text/plain"

src/file/unindexedfileiterator.cpp
142

This whole block belongs into a separate block, executed if (m_cTimeChanged).

This revision now requires changes to proceed.Jun 2 2019, 11:41 PM
poboiko updated this revision to Diff 59068.Jun 3 2019, 2:21 PM

Split test to three separate test functions, which cover different test cases.

Renamed folders to something more meaningful; put all the names as static
consts in the very beginning, for further reuse inside test functions.

Removed useless mimetype detection and use "text/plain" explicitly

poboiko updated this revision to Diff 59070.Jun 3 2019, 2:35 PM
poboiko marked 3 inline comments as done.

Moved m_nameChanged check inside separate block

poboiko added inline comments.Jun 3 2019, 2:35 PM
src/file/unindexedfileiterator.cpp
142

Did you mean m_mTimeChanged?

poboiko updated this revision to Diff 59071.Jun 3 2019, 2:37 PM

Fixed comment with directory structure

bruns added inline comments.Jun 3 2019, 5:46 PM
autotests/unit/file/unindexedfileiteratortest.cpp
99

Make this plain members, not pointers.
Also, one temporary dir is enough, you can put the db and the test tree side-by-side.

src/file/unindexedfileiterator.cpp
122

thats wrong:

$:/tmp> touch dir
$:/tmp> stat dir
  File: dir
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: 3fh/63d Inode: 1892961     Links: 1
Access: (0644/-rw-r--r--)  Uid: ( 1000/ sbruens)   Gid: (  100/   users)
Access: 2019-06-03 19:21:57.764626372 +0200
Modify: 2019-06-03 19:21:57.764626372 +0200
Change: 2019-06-03 19:21:57.764626372 +0200
 Birth: 2019-06-03 19:21:57.764626372 +0200
$:/tmp> mv dir  dir_renamed
$:/tmp> stat dir_renamed 
  File: dir_renamed
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: 3fh/63d Inode: 1892961     Links: 1
Access: (0644/-rw-r--r--)  Uid: ( 1000/ sbruens)   Gid: (  100/   users)
Access: 2019-06-03 19:21:57.764626372 +0200
Modify: 2019-06-03 19:21:57.764626372 +0200
Change: 2019-06-03 19:22:07.096601727 +0200
 Birth: 2019-06-03 19:21:57.764626372 +0200
130

The comment does not match - the mtime changes when something in the directory is modified, when the directory is renamed, the ctime changes.

142

obviously not ...

142

unrelated whitespace change

poboiko updated this revision to Diff 59099.Jun 3 2019, 11:09 PM
poboiko marked 5 inline comments as done.

Use single temp dir

Fixed mTime -> cTime

poboiko added inline comments.Jun 3 2019, 11:14 PM
autotests/unit/file/unindexedfileiteratortest.cpp
99

I wanted both DB and directory tree to be recreated from scratch for each test, since tests modify DB or rename folders, and thus can affect each other. That's why I made them to be pointers.

src/file/unindexedfileiterator.cpp
118

I wanted to note that this line is messed up a bit: either fileMTime should be renamed to fileCTime, or even removed, just like three lines above.
(it looks like the variable was introduced some time ago inside #ifdef QT < 5.10.0, which was removed recently with Qt dependency bump).

Since that's just some cosmetic change, I can change it in this patch as well. Or should I make a new one?

122

Right, sorry, I've mixed them up. Damn russian locale.

130

The comment is not mine - but that's precisely what it says. We don't want to reindex a directory every time something inside it modifies.

poboiko updated this revision to Diff 59951.Jun 16 2019, 6:17 PM

Rebase on master.

Since we now reindex folder always when cTime changes, revert nameChanged() routine
Cover the case when we feed UnIndexedFileIterator with path that ends with /

poboiko retitled this revision from [baloo_file] Index renamed folders inside UnindexedFileIndexer to [UnIndexedFileIteratorTest] Add tests.Jun 16 2019, 6:19 PM
poboiko edited the summary of this revision. (Show Details)