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

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

Details

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
fix-baloo-widget-tests
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11143
Build 11161: 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

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.Apr 24 2019, 6:38 AM
meven edited the summary of this revision. (Show Details)
meven edited the summary of this revision. (Show Details)Apr 24 2019, 6:43 AM
meven updated this revision to Diff 56864.Apr 24 2019, 6:57 AM

Added some comments about expected future update necessary.

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

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.Apr 24 2019, 6:58 AM
bruns added inline comments.Apr 24 2019, 12:45 PM
autotests/filemetadataitemcounttest.cpp
78

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.Apr 24 2019, 1:02 PM
autotests/filemetadataitemcounttest.cpp
78

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

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

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.Apr 25 2019, 10:28 AM
autotests/filemetadataitemcounttest.cpp
78

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.Apr 25 2019, 6:27 PM
autotests/filemetadataitemcounttest.cpp
78

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.May 6 2019, 11:04 AM
autotests/filemetadataitemcounttest.cpp
78

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

astippich accepted this revision.May 26 2019, 8:10 AM

I'm going to accept to fix the test for now, but it should be rewritten in the future

This revision is now accepted and ready to land.May 26 2019, 8:10 AM
This revision was automatically updated to reflect the committed changes.
meven added a comment.May 27 2019, 8:57 AM

I'm going to accept to fix the test for now, but it should be rewritten in the future

Thanks. I wish I had avoided the test break in the first place.

IMHO fixing a failing test is more important that refactoring a bad one, at least as a first step unless the test is so unstable it would require refactoring.