Code review comment for lp://staging/~osomon/webbrowser-app/systemwide-search-engines

> Running the new AP tests gives this error:
>
> Traceback (most recent call last):
> File "/home/nerochiaro/projects/phone/webbrowser-
> app/tests/autopilot/webbrowser_app/tests/test_settings.py", line 45, in
> test_open_close_searchengine_page
> self.assertThat(old_engine, NotEquals(""))
> File "/usr/lib/python3/dist-packages/testtools/testcase.py", line 423, in
> assertThat
> raise mismatch_error
> testtools.matchers._impl.MismatchError: '' == ''
>
> More info on my environment:
> I have one file in ~/.local/share/webbrowser-app/searchengines called bing.xml
> and completely empty
> I built the branch in-tree and I am running the tests from tests/autopilot

This test passes here, tests ran in the same conditions. Can you reliably reproduce the failure?

> This seems to indicate that the browser is reading the files from
> ~/.local/share/webbrowser-app which should not happen during tests.
>
> First verify why this makes the tests fail. I don't see why it should, so I
> suspect a bug somewhere.
>
> Second, we are testing by doing things in the current user directory. This is
> not safe. We should add a test mode command line switch to the app which calls
> QStandardPaths::setTestModeEnabled(true), so that QT will return special test
> paths for all standard paths. Then run our tests in this mode.

This will be addressed by https://code.launchpad.net/~osomon/webbrowser-app/autopilot-temp-profile/+merge/257490, although the implementation differs from your suggestion, but achieves the same result. It’s in a silo, awaiting QA verification for landing.

> Once that is done we should ideally set up more AP tests to verify the
> following features:
> - test that the names and descriptions of the engines correspond to what is
> read from the files

This is already unit-tested.

> - test overriding an engine by placing a file of the same name, and verifying
> that the search url, description and name are correctly overriden
> - test overriding an engine by placing an empty file of the same name, then
> verifying that the engine is removed

I’ll add unit tests for the above two suggestions, thanks.

> - verify a few of the above by manipulating the files in the list while app is
> running and verifying that the settings page picks these up the next time it
> is opened.

Although a valid test, that sounds a bit overkill for an autopilot test. I’ll see if I can write a QML test for it though.