[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 12910
Build 12928: 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
126–146

Bad variable name, these are files and dirs.

130

bad name, use something like excluded.

166

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

src/file/unindexedfileiterator.cpp
126 ↗(On Diff #58934)

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
126 ↗(On Diff #58934)

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
100

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 ↗(On Diff #59071)

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 ↗(On Diff #59071)

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

142 ↗(On Diff #59071)

unrelated whitespace change

126 ↗(On Diff #58934)

obviously not ...

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
100

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 ↗(On Diff #59071)

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 ↗(On Diff #59071)

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

130 ↗(On Diff #59071)

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)