Fix test, taking into account access time field is now always available
Needs ReviewPublic

Authored by meven on Apr 23 2019, 7:45 AM.

Details

Reviewers
astippich
bruns
Summary

The creation date time field has been introduced but still depends further on kio 5.58+, glibc 2.28+, linux 4.11+ and compatible filesystem (ext4 with 256 bits inodes for instance, btrfs).

Diff Detail

Repository
R824 Baloo Widgets
Branch
arcpatch-D20763
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11169
Build 11187: arc lint + arc unit
meven created this revision.Apr 23 2019, 7:45 AM
Restricted Application added a project: Baloo. · View Herald TranscriptApr 23 2019, 7:45 AM
Restricted Application added a subscriber: Baloo. · View Herald Transcript
meven requested review of this revision.Apr 23 2019, 7:45 AM
meven updated this revision to Diff 56815.Apr 23 2019, 12:07 PM

Removing bad modifications

I think the description in the parenthesis should be moved into the summary.

autotests/filemetadataitemcounttest.cpp
74–77

If I understand correctly, this needs to be increased when glibc 2.28, linux 4.11+ and compatible fs are available? Is this easily detectable?
If not, please add a comment here describing this issue.

meven retitled this revision from Fix test, taking into account access time field is now always available (creation date time still depends on glibc 2.28, linux 4.11+ and compatible fs) to Fix test, taking into account access time field is now always available.Wed, Apr 24, 6:38 AM
meven edited the summary of this revision. (Show Details)
meven edited the summary of this revision. (Show Details)Wed, Apr 24, 6:43 AM
meven updated this revision to Diff 56864.Wed, Apr 24, 6:57 AM

Added some comments about expected future update necessary.

meven marked an inline comment as done.Wed, Apr 24, 6:58 AM
meven added inline comments.
autotests/filemetadataitemcounttest.cpp
74–77

It is not easily detectable AFAIK, or at least we would not want to have a detection code about it.
Letting the test fail again in the future is to me the best course of action
A comment in the code to point out the issue to whoever stumbles upon the test failure should do the trick.

meven marked 2 inline comments as done.Wed, Apr 24, 6:58 AM
bruns added inline comments.Wed, Apr 24, 12:45 PM
autotests/filemetadataitemcounttest.cpp
81

Currently, the test covers both the filemetadataprovider and the filemetadatawidget itself. We should inject suitable data here instead, bypassing the provider.

meven added inline comments.Wed, Apr 24, 1:02 PM
autotests/filemetadataitemcounttest.cpp
81

The provider has a constant file being, see line below. Maybe you missed it or I missed your point.

bruns added inline comments.Wed, Apr 24, 4:20 PM
autotests/filemetadataitemcounttest.cpp
81

Yes, you missed it ...

The test relies on the provider supplying correct data, which it can not do due to platform differences.

instead of m_widget->setItems(...), which triggers the provider to fetch the data from the file, we can inject the data. For this, we need a mock provider.

meven added inline comments.Thu, Apr 25, 10:28 AM
autotests/filemetadataitemcounttest.cpp
81

I feel mocking the provider here would be pointless, the test would just check that 2 widgets are created for each field that is passed to it.

The current test checks that for the field passed 20 fields are extracted and have 2 widgets in the FileMetaDataWidget.
This check was quite relevant since it was the only one that broke because of 5f2ab5c62dd56e5a1dae468355c9e72d84d94398.

So you would tell, let's add a provider test, to which I would respond the current test is already covering this functionality.

The test coverage is not great, I will give you that but I want to point out this review is about fixing a test regression introduced by 5f2ab5c62dd56e5a1dae468355c9e72d84d94398 in the first place.

bruns added inline comments.Thu, Apr 25, 6:27 PM
autotests/filemetadataitemcounttest.cpp
81

So you would tell, let's add a provider test, to which I would respond the current test is already covering this functionality.

Thats the reason it is called a unit test - test one functional unit at a time.

The widget mangles the QVariantMap entries from the provider in various ways. The map is sorted, the type of the value widget depends on the key.

The provider creates a QVariantMap, which can be checked much better standalone than via the widget. The current test passes even when the values are complete garbage.

meven added inline comments.Mon, May 6, 11:04 AM
autotests/filemetadataitemcounttest.cpp
81

I feel we should fix the test failure before anything.
I did here only intend to fix the test.