More sql code review fixes.

- remove outdated comment
- fix sql init error message
- clean up history text import code
- fix test_history file path in coverage check
- use real web history, not stub, for completion model tests
- use qtmodeltester in sql/list_category tests
- test url encoding in history tests
- fix test_clear by using a callable mock
- remove test_debug_dump_history_oserror as the check is now the same as
  for the file not existing
- rename nonempty to data in test_completionmodel
- add more delete_cur_item tests
- test empty option/value completion

qutebrowser/app.py

@@ -83,8 +83,6 @@ def run(args):
     standarddir.init(args)
 
     if args.version:
-        # we need to init sql to print the sql version
-        # we can use an in-memory database as we just want to query the version
         print(version.version())
         sys.exit(usertypes.Exit.ok)
 
@@ -431,8 +429,8 @@ def _init_modules(args, crash_handler):
     try:
         sql.init(os.path.join(standarddir.data(), 'history.sqlite'))
     except sql.SqlException as e:
-        error.handle_fatal_exc(e, args, 'Is sqlite installed?',
+        error.handle_fatal_exc(e, args, 'Error initializing SQL',
-                               pre_text='Failed to initialize SQL')
+                               pre_text='Error initializing SQL')
         sys.exit(usertypes.Exit.err_init)
 
     log.init.debug("Initializing web history...")

qutebrowser/browser/history.py

@@ -171,6 +171,8 @@ class WebHistory(sql.SqlTable):
 
     def _parse_entry(self, line):
         """Parse a history line like '12345 http://example.com title'."""
+        if not line or line.startswith('#'):
+            return None
         data = line.split(maxsplit=2)
         if len(data) == 2:
             atime, url = data
@@ -240,11 +242,8 @@ class WebHistory(sql.SqlTable):
             data = {'url': [], 'title': [], 'atime': [], 'redirect': []}
             completion_data = {'url': [], 'title': [], 'last_atime': []}
             for (i, line) in enumerate(f):
-                line = line.strip()
-                if not line or line.startswith('#'):
-                    continue
                 try:
-                    parsed = self._parse_entry(line)
+                    parsed = self._parse_entry(line.strip())
                     if parsed is None:
                         continue
                     url, title, atime, redirect = parsed
@@ -271,10 +270,6 @@ class WebHistory(sql.SqlTable):
         """
         dest = os.path.expanduser(dest)
 
-        dirname = os.path.dirname(dest)
-        if not os.path.exists(dirname):
-            raise cmdexc.CommandError('Path does not exist', dirname)
-
         lines = ('{}{} {} {}'
                  .format(int(x.atime), '-r' * x.redirect, x.url, x.title)
                  for x in self.select(sort_by='atime', sort_order='asc'))

scripts/dev/check_coverage.py

@@ -51,12 +51,10 @@ PERFECT_FILES = [
         'browser/webkit/cache.py'),
     ('tests/unit/browser/webkit/test_cookies.py',
         'browser/webkit/cookies.py'),
-    ('tests/unit/browser/webkit/test_history.py',
+    ('tests/unit/browser/test_history.py',
         'browser/history.py'),
-    ('tests/unit/browser/webkit/test_history.py',
+    ('tests/unit/browser/test_history.py',
         'browser/webkit/webkithistory.py'),
-    ('tests/unit/browser/webkit/test_history.py',
-        'browser/history.py'),
     ('tests/unit/browser/webkit/http/test_http.py',
         'browser/webkit/http.py'),
     ('tests/unit/browser/webkit/http/test_content_disposition.py',

tests/helpers/fixtures.py

@@ -257,15 +257,6 @@ def bookmark_manager_stub(stubs):
     objreg.delete('bookmark-manager')
 
 
-@pytest.fixture
-def web_history_stub(init_sql, stubs):
-    """Fixture which provides a fake web-history object."""
-    stub = stubs.WebHistoryStub()
-    objreg.register('web-history', stub)
-    yield stub
-    objreg.delete('web-history')
-
-
 @pytest.fixture
 def session_manager_stub(stubs):
     """Fixture which provides a fake session-manager object."""
@@ -491,3 +482,22 @@ def init_sql(data_tmpdir):
     sql.init(path)
     yield
     sql.close()
+
+
+@pytest.fixture
+def validate_model(qtmodeltester):
+    """Provides a function to validate a completion category."""
+    def validate(cat, expected):
+        """Check that a category contains the items in the given order.
+
+        Args:
+            cat: The category to inspect.
+            expected: A list of tuples containing the expected items.
+        """
+        qtmodeltester.data_display_may_return_none = True
+        qtmodeltester.check(cat)
+        assert cat.rowCount() == len(expected)
+        for row, items in enumerate(expected):
+            for col, item in enumerate(items):
+                assert cat.data(cat.index(row, col)) == item
+    return validate

tests/helpers/stubs.py

@@ -33,7 +33,6 @@ from qutebrowser.browser import browsertab
 from qutebrowser.config import configexc
 from qutebrowser.utils import usertypes, utils
 from qutebrowser.mainwindow import mainwindow
-from qutebrowser.misc import sql
 
 
 class FakeNetworkCache(QAbstractNetworkCache):
@@ -523,37 +522,6 @@ class QuickmarkManagerStub(UrlMarkManagerStub):
         self.delete(key)
 
 
-class WebHistoryStub(sql.SqlTable):
-
-    """Stub for the web-history object."""
-
-    def __init__(self):
-        super().__init__("History", ['url', 'title', 'atime', 'redirect'])
-        self.completion = sql.SqlTable("CompletionHistory",
-                                       ['url', 'title', 'last_atime'])
-
-    def __contains__(self, url):
-        q = self.contains_query('url')
-        return q.run(val=url).value()
-
-    def add_url(self, url, title="", *, redirect=False, atime=None):
-        self.insert({'url': url, 'title': title, 'atime': atime,
-                     'redirect': redirect})
-        if not redirect:
-            self.completion.insert({'url': url,
-                                    'title': title,
-                                    'last_atime': atime})
-
-    def delete_url(self, url):
-        """Remove all history entries with the given url.
-
-        Args:
-            url: URL string to delete.
-        """
-        self.delete('url', url)
-        self.completion.delete('url', url)
-
-
 class HostBlockerStub:
 
     """Stub for the host-blocker object."""

tests/helpers/utils.py

@@ -170,16 +170,3 @@ def abs_datapath():
     """Get the absolute path to the end2end data directory."""
     file_abs = os.path.abspath(os.path.dirname(__file__))
     return os.path.join(file_abs, '..', 'end2end', 'data')
-
-
-def validate_model(cat, expected):
-    """Check that a category contains the expected items in the given order.
-
-    Args:
-        cat: The category to inspect.
-        expected: A list of tuples containing the expected items.
-    """
-    assert cat.rowCount() == len(expected)
-    for row, items in enumerate(expected):
-        for col, item in enumerate(items):
-            assert cat.data(cat.index(row, col)) == item

tests/unit/browser/test_history.py

@@ -21,6 +21,7 @@
 
 import logging
 import os
+import unittest.mock
 
 import pytest
 from PyQt5.QtCore import QUrl
@@ -114,7 +115,7 @@ def test_clear(qtbot, tmpdir, hist, mocker):
     hist.add_url(QUrl('http://www.qutebrowser.org/'))
 
     m = mocker.patch('qutebrowser.browser.history.message.confirm_async',
-                     spec=[])
+                     new=unittest.mock.Mock, spec=[])
     hist.clear()
     assert m.called
 
@@ -134,16 +135,22 @@ def test_delete_url(hist):
     before = set(hist)
     hist.delete_url(QUrl('http://example.com/1'))
     diff = before.difference(set(hist))
-    assert diff == set([('http://example.com/1', '', 0, False)])
+    assert diff == {('http://example.com/1', '', 0, False)}
 
 
-@pytest.mark.parametrize('url, atime, title, redirect', [
+@pytest.mark.parametrize('url, atime, title, redirect, expected_url', [
-    ('http://www.example.com', 12346, 'the title', False),
+    ('http://www.example.com', 12346, 'the title', False,
-    ('http://www.example.com', 12346, 'the title', True)
+        'http://www.example.com'),
+    ('http://www.example.com', 12346, 'the title', True,
+        'http://www.example.com'),
+    ('http://www.example.com/spa ce', 12346, 'the title', False,
+        'http://www.example.com/spa%20ce'),
+    ('https://user:pass@example.com', 12346, 'the title', False,
+        'https://user@example.com'),
 ])
-def test_add_item(qtbot, hist, url, atime, title, redirect):
+def test_add_item(qtbot, hist, url, atime, title, redirect, expected_url):
     hist.add_url(QUrl(url), atime=atime, title=title, redirect=redirect)
-    assert list(hist) == [(url, title, atime, redirect)]
+    assert list(hist) == [(expected_url, title, atime, redirect)]
 
 
 def test_add_item_invalid(qtbot, hist, caplog):
@@ -164,7 +171,7 @@ def test_add_item_invalid(qtbot, hist, caplog):
 def test_add_from_tab(hist, level, url, req_url, expected, mock_time, caplog):
     with caplog.at_level(level):
         hist.add_from_tab(QUrl(url), QUrl(req_url), 'title')
-    assert set(list(hist)) == set(expected)
+    assert set(hist) == set(expected)
 
 
 @pytest.fixture
@@ -330,11 +337,3 @@ def test_debug_dump_history_nonexistent(hist, tmpdir):
     histfile = tmpdir / 'nonexistent' / 'history'
     with pytest.raises(cmdexc.CommandError):
         hist.debug_dump_history(str(histfile))
-
-
-def test_debug_dump_history_oserror(hist, tmpdir):
-    histfile = tmpdir / 'history'
-    histfile.write('')
-    os.chmod(str(histfile), 0)
-    with pytest.raises(cmdexc.CommandError):
-        hist.debug_dump_history(str(histfile))

tests/unit/completion/test_completionmodel.py

@@ -39,18 +39,18 @@ def test_first_last_item(counts):
         cat = mock.Mock(spec=['layoutChanged'])
         cat.rowCount = mock.Mock(return_value=c, spec=[])
         model.add_category(cat)
-    nonempty = [i for i, rowCount in enumerate(counts) if rowCount > 0]
+    data = [i for i, rowCount in enumerate(counts) if rowCount > 0]
-    if not nonempty:
+    if not data:
         # with no items, first and last should be an invalid index
         assert not model.first_item().isValid()
         assert not model.last_item().isValid()
     else:
-        first = nonempty[0]
+        first = data[0]
-        last = nonempty[-1]
+        last = data[-1]
-        # first item of the first nonempty category
+        # first item of the first data category
         assert model.first_item().row() == 0
         assert model.first_item().parent().row() == first
-        # last item of the last nonempty category
+        # last item of the last data category
         assert model.last_item().row() == counts[last] - 1
         assert model.last_item().parent().row() == last
 

tests/unit/completion/test_listcategory.py

@@ -19,10 +19,12 @@
 
 """Tests for CompletionFilterModel."""
 
+from unittest import mock
+
 import pytest
 
-from helpers import utils
 from qutebrowser.completion.models import listcategory
+from qutebrowser.commands import cmdexc
 
 
 @pytest.mark.parametrize('pattern, before, after', [
@@ -43,8 +45,26 @@ from qutebrowser.completion.models import listcategory
      [('foo', 'bar'), ('bar', 'foo'), ('bar', 'bar')],
      [('foo', 'bar'), ('bar', 'foo')]),
 ])
-def test_set_pattern(pattern, before, after):
+def test_set_pattern(pattern, before, after, validate_model):
     """Validate the filtering and sorting results of set_pattern."""
     cat = listcategory.ListCategory('Foo', before)
     cat.set_pattern(pattern)
-    utils.validate_model(cat, after)
+    validate_model(cat, after)
+
+
+def test_delete_cur_item(validate_model):
+    func = mock.Mock(spec=[])
+    cat = listcategory.ListCategory('Foo', [('a', 'b'), ('c', 'd')],
+                                    delete_func=func)
+    idx = cat.index(0, 0)
+    cat.delete_cur_item(idx)
+    func.assert_called_once_with(['a', 'b'])
+    validate_model(cat, [('c', 'd')])
+
+
+def test_delete_cur_item_no_func(validate_model):
+    cat = listcategory.ListCategory('Foo', [('a', 'b'), ('c', 'd')])
+    idx = cat.index(0, 0)
+    with pytest.raises(cmdexc.CommandError, match="Cannot delete this item."):
+        cat.delete_cur_item(idx)
+    validate_model(cat, [('a', 'b'), ('c', 'd')])

tests/unit/completion/test_models.py

@@ -27,7 +27,8 @@ from PyQt5.QtCore import QUrl
 
 from qutebrowser.completion.models import miscmodels, urlmodel, configmodel
 from qutebrowser.config import sections, value
-from qutebrowser.misc import sql
+from qutebrowser.utils import objreg
+from qutebrowser.browser import history
 
 
 def _check_completions(model, expected):
@@ -136,29 +137,33 @@ def bookmarks(bookmark_manager_stub):
 
 
 @pytest.fixture
-def history_completion_table(init_sql):
+def web_history(init_sql, stubs):
-    return sql.SqlTable("CompletionHistory", ['url', 'title', 'last_atime'])
+    """Fixture which provides a web-history object."""
+    stub = history.WebHistory()
+    objreg.register('web-history', stub)
+    yield stub
+    objreg.delete('web-history')
 
 
 @pytest.fixture
-def web_history(web_history_stub):
+def web_history_populated(web_history):
     """Pre-populate the web-history database."""
-    web_history_stub.add_url(
+    web_history.add_url(
-        url='http://qutebrowser.org',
+        url=QUrl('http://qutebrowser.org'),
         title='qutebrowser',
         atime=datetime(2015, 9, 5).timestamp()
     )
-    web_history_stub.add_url(
+    web_history.add_url(
-        url='https://python.org',
+        url=QUrl('https://python.org'),
         title='Welcome to Python.org',
         atime=datetime(2016, 3, 8).timestamp()
     )
-    web_history_stub.add_url(
+    web_history.add_url(
-        url='https://github.com',
+        url=QUrl('https://github.com'),
         title='https://github.com',
         atime=datetime(2016, 5, 1).timestamp()
     )
-    return web_history_stub
+    return web_history
 
 
 def test_command_completion(qtmodeltester, monkeypatch, stubs, config_stub,
@@ -261,8 +266,8 @@ def test_bookmark_completion(qtmodeltester, bookmarks):
     })
 
 
-def test_url_completion(qtmodeltester, config_stub, web_history, quickmarks,
+def test_url_completion(qtmodeltester, config_stub, web_history_populated,
-                        bookmarks):
+                        quickmarks, bookmarks):
     """Test the results of url completion.
 
     Verify that:
@@ -313,12 +318,12 @@ def test_url_completion(qtmodeltester, config_stub, web_history, quickmarks,
     ('foo%bar', '', '%', 1),
     ('foobar', '', '%', 0),
 ])
-def test_url_completion_pattern(config_stub, web_history_stub,
+def test_url_completion_pattern(config_stub, web_history,
                                 quickmark_manager_stub, bookmark_manager_stub,
                                 url, title, pattern, rowcount):
     """Test that url completion filters by url and title."""
     config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d'}
-    web_history_stub.add_url(url, title)
+    web_history.add_url(QUrl(url), title)
     model = urlmodel.url()
     model.set_pattern(pattern)
     # 2, 0 is History
@@ -349,7 +354,7 @@ def test_url_completion_delete_bookmark(qtmodeltester, config_stub, bookmarks,
 
 
 def test_url_completion_delete_quickmark(qtmodeltester, config_stub,
-                                         web_history, quickmarks, bookmarks,
+                                         quickmarks, web_history, bookmarks,
                                          qtbot):
     """Test deleting a bookmark from the url completion model."""
     config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d'}
@@ -373,7 +378,7 @@ def test_url_completion_delete_quickmark(qtmodeltester, config_stub,
 
 
 def test_url_completion_delete_history(qtmodeltester, config_stub,
-                                       web_history_stub, web_history,
+                                       web_history_populated,
                                        quickmarks, bookmarks):
     """Test deleting a history entry."""
     config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d'}
@@ -389,9 +394,9 @@ def test_url_completion_delete_history(qtmodeltester, config_stub,
     assert model.data(parent) == "History"
     assert model.data(idx) == 'https://python.org'
 
-    assert 'https://python.org' in web_history_stub
+    assert 'https://python.org' in web_history_populated
     model.delete_cur_item(idx)
-    assert 'https://python.org' not in web_history_stub
+    assert 'https://python.org' not in web_history_populated
 
 
 def test_session_completion(qtmodeltester, session_manager_stub):
@@ -504,6 +509,12 @@ def test_setting_option_completion(qtmodeltester, monkeypatch, stubs,
     })
 
 
+def test_setting_option_completion_empty(monkeypatch, stubs, config_stub):
+    module = 'qutebrowser.completion.models.configmodel'
+    _patch_configdata(monkeypatch, stubs, module + '.configdata.DATA')
+    assert configmodel.option('typo') is None
+
+
 def test_setting_option_completion_valuelist(qtmodeltester, monkeypatch, stubs,
                                              config_stub):
     module = 'qutebrowser.completion.models.configmodel'
@@ -545,6 +556,13 @@ def test_setting_value_completion(qtmodeltester, monkeypatch, stubs,
     })
 
 
+def test_setting_value_completion_empty(monkeypatch, stubs, config_stub):
+    module = 'qutebrowser.completion.models.configmodel'
+    _patch_configdata(monkeypatch, stubs, module + '.configdata.DATA')
+    config_stub.data = {'general': {}}
+    assert configmodel.value('general', 'typo') is None
+
+
 def test_bind_completion(qtmodeltester, monkeypatch, stubs, config_stub,
                          key_config_stub):
     """Test the results of keybinding command completion.
@@ -583,7 +601,7 @@ def test_bind_completion(qtmodeltester, monkeypatch, stubs, config_stub,
 def test_url_completion_benchmark(benchmark, config_stub,
                                   quickmark_manager_stub,
                                   bookmark_manager_stub,
-                                  web_history_stub):
+                                  web_history):
     """Benchmark url completion."""
     config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d',
                                       'web-history-max-items': 1000}
@@ -595,7 +613,7 @@ def test_url_completion_benchmark(benchmark, config_stub,
         'title': ['title{}'.format(i) for i in r]
     }
 
-    web_history_stub.completion.insert_batch(entries)
+    web_history.completion.insert_batch(entries)
 
     quickmark_manager_stub.marks = collections.OrderedDict([
         ('title{}'.format(i), 'example.com/{}'.format(i))

tests/unit/completion/test_sqlcategory.py

@@ -23,7 +23,6 @@ import unittest.mock
 
 import pytest
 
-from helpers import utils
 from qutebrowser.misc import sql
 from qutebrowser.completion.models import sqlcategory
 from qutebrowser.commands import cmdexc
@@ -61,14 +60,14 @@ pytestmark = pytest.mark.usefixtures('init_sql')
      [('B', 'C', 2), ('A', 'F', 0), ('C', 'A', 1)],
      [('B', 'C', 2), ('C', 'A', 1), ('A', 'F', 0)]),
 ])
-def test_sorting(sort_by, sort_order, data, expected):
+def test_sorting(sort_by, sort_order, data, expected, validate_model):
     table = sql.SqlTable('Foo', ['a', 'b', 'c'])
     for row in data:
         table.insert({'a': row[0], 'b': row[1], 'c': row[2]})
     cat = sqlcategory.SqlCategory('Foo', filter_fields=['a'], sort_by=sort_by,
                                   sort_order=sort_order)
     cat.set_pattern('')
-    utils.validate_model(cat, expected)
+    validate_model(cat, expected)
 
 
 @pytest.mark.parametrize('pattern, filter_cols, before, after', [
@@ -116,7 +115,7 @@ def test_sorting(sort_by, sort_order, data, expected):
      [("can't touch this", '', ''), ('a', '', '')],
      [("can't touch this", '', '')]),
 ])
-def test_set_pattern(pattern, filter_cols, before, after):
+def test_set_pattern(pattern, filter_cols, before, after, validate_model):
     """Validate the filtering and sorting results of set_pattern."""
     table = sql.SqlTable('Foo', ['a', 'b', 'c'])
     for row in before:
@@ -124,15 +123,15 @@ def test_set_pattern(pattern, filter_cols, before, after):
     filter_fields = [['a', 'b', 'c'][i] for i in filter_cols]
     cat = sqlcategory.SqlCategory('Foo', filter_fields=filter_fields)
     cat.set_pattern(pattern)
-    utils.validate_model(cat, after)
+    validate_model(cat, after)
 
 
-def test_select():
+def test_select(validate_model):
     table = sql.SqlTable('Foo', ['a', 'b', 'c'])
     table.insert({'a': 'foo', 'b': 'bar', 'c': 'baz'})
     cat = sqlcategory.SqlCategory('Foo', filter_fields=['a'], select='b, c, a')
     cat.set_pattern('')
-    utils.validate_model(cat, [('bar', 'baz', 'foo')])
+    validate_model(cat, [('bar', 'baz', 'foo')])
 
 
 def test_delete_cur_item():