Code review comment for lp:~mhr3/unity-scopes-shell/scopes-ng-tests

92 + return QVariant(QString::fromStdString(v.get_string()));
107 + return QVariant(QString::fromStdString(result->category()->id()));

You don't need to write the QVariant there (and all the following lines) explicity. Obviously it doesn't really hurt in terms of performance but imho it hurts readability. You don't gain anything in terms of converting anyways by writing it explicitly anyways.

=====
168 +#include <unordered_map>
210 + std::unordered_map<std::string, std::string> m_componentMapping;

unused?

========

379 +#Scope.GroupConfigDir = @CMAKE_CURRENT_BINARY_DIR@/scope-groups

as this is commented I gotta ask: Is this needed at all? in the future? do we perhaps need a FIXME comment?

======
418 === added file 'tests/data/scope-A/scope-A.cpp'

This seems like a mock. calling the directory "mocks" instead of data would make that more obvious imho.

=======
674 + void testTwoSearches()
675 + {
676 + QCOMPARE(m_scope->searchInProgress(), false);
677 + // perform a search
678 + m_scope->setSearchQuery(QString(""));
679 + QCOMPARE(m_scope->searchInProgress(), true);
680 + QTRY_COMPARE(m_scope->searchInProgress(), false);
681 +
682 + // ensure categories have > 0 rows
683 + auto categories = m_scope->categories();
684 + auto categories_count = categories->rowCount();
685 + QVERIFY(categories_count > 0);
686 +
687 + m_scope->setSearchQuery(QString("foo"));
688 + QCOMPARE(m_scope->searchInProgress(), true);
689 + // wait for the search to finish
690 + QTRY_COMPARE(m_scope->searchInProgress(), false);
691 +
692 + // shouldn't create more nor fewer categories
693 + QVERIFY(categories->rowCount() == categories_count);
694 + }

Wouldn't it make sense to use data columns/rows? This way we can just add new rows to test different things like uppercase, lowercase, special chars etc for search strings (and it's even fewer code than what you have now)

Ok. I guess the testTwoSearches is intentionally performing 2 searches in a row to see if it recovers from searches. Still, there might be issues in different search strings and I really think we should have such a data driven test case to easily be able to add new search terms to the tests. Maybe it makes more sense to transform the testBasicResultData into such a thing. Maybe both? You decide.

======

I really don't like the overuse of the "auto" type. Imho it makes code hard to read.