Differential D20763 Diff 56864 autotests/filemetadataitemcounttest.cpp

Changeset View

Standalone View

autotests/filemetadataitemcounttest.cpp

Show First 20 Lines • Show All 65 Lines • ▼ Show 20 Line(s)
66		66
67	void FileMetadataItemCountTest::cleanup()	67		void FileMetadataItemCountTest::cleanup()
68	{	68		{
69	delete m_widget;	69		delete m_widget;
70	}	70		}
71		71
72	void FileMetadataItemCountTest::testItemCount()	72		void FileMetadataItemCountTest::testItemCount()
73	{	73		{
74	const int expectedItems = 19;	74		// the number of items will increase in the future adding the file creation time field
		75		// when the system has KIO 5.58, glibc 2.28, linux 4.11 and a filesystem storing file creation times (btrfs, ext4...)
		76		// The expectedItems count will need to be updated
		77		const int expectedItems = 20;
			astippichUnsubmitted Done 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. astippich: If I understand correctly, this needs to be increased when glibc 2.28, linux 4.11+ and…
			mevenAuthorUnsubmitted Done 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: It is not easily detectable AFAIK, or at least we would not want to have a detection code about…
75	const int widgetsPerItem = 2;	78		const int widgetsPerItem = 2;
76		79
77	QSignalSpy spy(m_widget, &Baloo::FileMetaDataWidget::metaDataRequestFinished);	80		QSignalSpy spy(m_widget, &Baloo::FileMetaDataWidget::metaDataRequestFinished);
78	m_widget->setItems(KFileItemList()	81		m_widget->setItems(KFileItemList()
			brunsUnsubmitted Not Done Currently, the test covers both the filemetadataprovider and the filemetadatawidget itself. We should inject suitable data here instead, bypassing the provider. bruns: Currently, the test covers both the filemetadataprovider and the filemetadatawidget itself. We…
			mevenAuthorUnsubmitted Not Done The provider has a constant file being, see line below. Maybe you missed it or I missed your point. meven: The provider has a constant file being, see line below. Maybe you missed it or I missed your…
			brunsUnsubmitted Not Done 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. bruns: Yes, you missed it ... The test relies on the provider supplying correct data, which it can…
			mevenAuthorUnsubmitted Not Done 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. meven: I feel mocking the provider here would be pointless, the test would just check that 2 widgets…
			brunsUnsubmitted Not Done 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. bruns: > So you would tell, let's add a provider test, to which I would respond the current test is…
			mevenAuthorUnsubmitted Not Done I feel we should fix the test failure before anything. I did here only intend to fix the test. meven: I feel we should fix the test failure before anything. I did here only intend to fix the test.
79	<< QUrl::fromLocalFile(QFINDTESTDATA("samplefiles/testtagged.mp3"))	82		<< QUrl::fromLocalFile(QFINDTESTDATA("samplefiles/testtagged.mp3"))
80	);	83		);
81		84
82	QVERIFY(spy.wait());	85		QVERIFY(spy.wait());
83	QCOMPARE(spy.count(), 1);	86		QCOMPARE(spy.count(), 1);
84		87
85	QList<QWidget> items = m_widget->findChildren<QWidget>(QString(), Qt::FindDirectChildrenOnly);	88		QList<QWidget> items = m_widget->findChildren<QWidget>(QString(), Qt::FindDirectChildrenOnly);
86	QCOMPARE(items.count(), expectedItems * widgetsPerItem);	89		QCOMPARE(items.count(), expectedItems * widgetsPerItem);
87		90
88	}	91		}