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).
Details
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
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? |
autotests/filemetadataitemcounttest.cpp | ||
---|---|---|
74 | It is not easily detectable AFAIK, or at least we would not want to have a detection code about it. |
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. |
autotests/filemetadataitemcounttest.cpp | ||
---|---|---|
78 | The provider has a constant file being, see line below. Maybe you missed it or I missed your point. |
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. |
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. 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. |
autotests/filemetadataitemcounttest.cpp | ||
---|---|---|
78 |
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. |
autotests/filemetadataitemcounttest.cpp | ||
---|---|---|
78 | I feel we should fix the test failure before anything. |
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.