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

> 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.

Changed to do implicit conversions.

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

Will be used soon :)

> 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?

Removed.

> 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.

The data dir has all test configuration data files, plus the scope itself is really a "data" provider :) But I at least renamed the scope itself to mock-scope.

> 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.

The thing is, I'm not really using the various search strings to just change the data the scope gives you, it's mostly used to change behaviour of the search.

Maybe the problem really is with the naming of the test file, it's not just testing results (that's really just the last step), it's testing end to end communication with a real scope - that's why these four tiny test methods are able to cover 75% of the entire codebase.

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

Not sure what to do about that, it's convenient to write it that way :) Maybe it's again a naming issue, the variable name should be expressive enough to convey what it is.