From 1e1335aa5edf72076a9e25fd129d6e074cba4af6 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Mon, 3 Jul 2017 08:20:31 -0400 Subject: [PATCH] Make various SQL code review changes. - Fix outdated comments - Use mock specs when possible - More precise error message check in test_import_txt_invalid. - Fix copyright message - Tweak missing pyqt error message - Dead code: remove group_by and where from sqlcategory. With the new separate completion table, these are no longer used. - Move test_history out of webkit/. History is no longer purely webkit related, it could be webengine. --- qutebrowser/browser/qutescheme.py | 1 - qutebrowser/completion/models/listcategory.py | 10 ++++---- qutebrowser/completion/models/sqlcategory.py | 7 +----- qutebrowser/misc/earlyinit.py | 7 +----- qutebrowser/misc/sql.py | 2 +- .../unit/browser/{webkit => }/test_history.py | 11 ++++++--- tests/unit/completion/test_completionmodel.py | 11 +++++---- .../unit/completion/test_completionwidget.py | 6 ++--- tests/unit/completion/test_listcategory.py | 2 +- tests/unit/completion/test_sqlcategory.py | 23 +------------------ tests/unit/misc/test_lineparser.py | 3 ++- 11 files changed, 29 insertions(+), 54 deletions(-) rename tests/unit/browser/{webkit => }/test_history.py (97%) diff --git a/qutebrowser/browser/qutescheme.py b/qutebrowser/browser/qutescheme.py index 0c495d28b..e25b7df07 100644 --- a/qutebrowser/browser/qutescheme.py +++ b/qutebrowser/browser/qutescheme.py @@ -194,7 +194,6 @@ def history_data(start_time, offset=None): """ hist = objreg.get('web-history') if offset is not None: - # end is 24hrs earlier than start entries = hist.entries_before(start_time, limit=1000, offset=offset) else: # end is 24hrs earlier than start diff --git a/qutebrowser/completion/models/listcategory.py b/qutebrowser/completion/models/listcategory.py index ee9a5e53e..187ebcad6 100644 --- a/qutebrowser/completion/models/listcategory.py +++ b/qutebrowser/completion/models/listcategory.py @@ -36,7 +36,7 @@ class ListCategory(QSortFilterProxyModel): super().__init__(parent) self.name = name self.srcmodel = QStandardItemModel(parent=self) - self.pattern = '' + self._pattern = '' # ListCategory filters all columns self.columns_to_filter = [0, 1, 2] self.setFilterKeyColumn(-1) @@ -51,7 +51,7 @@ class ListCategory(QSortFilterProxyModel): Args: val: The value to set. """ - self.pattern = val + self._pattern = val val = re.sub(r' +', r' ', val) # See #1919 val = re.escape(val) val = val.replace(r'\ ', '.*') @@ -64,7 +64,7 @@ class ListCategory(QSortFilterProxyModel): def lessThan(self, lindex, rindex): """Custom sorting implementation. - Prefers all items which start with self.pattern. Other than that, uses + Prefers all items which start with self._pattern. Other than that, uses normal Python string sorting. Args: @@ -80,8 +80,8 @@ class ListCategory(QSortFilterProxyModel): left = self.srcmodel.data(lindex) right = self.srcmodel.data(rindex) - leftstart = left.startswith(self.pattern) - rightstart = right.startswith(self.pattern) + leftstart = left.startswith(self._pattern) + rightstart = right.startswith(self._pattern) if leftstart and rightstart: return left < right diff --git a/qutebrowser/completion/models/sqlcategory.py b/qutebrowser/completion/models/sqlcategory.py index 001a844b0..4819e940b 100644 --- a/qutebrowser/completion/models/sqlcategory.py +++ b/qutebrowser/completion/models/sqlcategory.py @@ -33,7 +33,7 @@ class SqlCategory(QSqlQueryModel): """Wraps a SqlQuery for use as a completion category.""" def __init__(self, name, *, title=None, filter_fields, sort_by=None, - sort_order=None, select='*', where=None, group_by=None, + sort_order=None, select='*', delete_func=None, parent=None): """Create a new completion category backed by a sql table. @@ -42,7 +42,6 @@ class SqlCategory(QSqlQueryModel): title: Title of category, defaults to table name. filter_fields: Names of fields to apply filter pattern to. select: A custom result column expression for the select statement. - where: An optional clause to filter out some rows. sort_by: The name of the field to sort by, or None for no sorting. sort_order: Either 'asc' or 'desc', if sort_by is non-None delete_func: Callback to delete a selected item. @@ -57,10 +56,6 @@ class SqlCategory(QSqlQueryModel): for f in filter_fields) querystr += ')' - if where: - querystr += ' and ' + where - if group_by: - querystr += ' group by {}'.format(group_by) if sort_by: assert sort_order in ['asc', 'desc'], sort_order querystr += ' order by {} {}'.format(sort_by, sort_order) diff --git a/qutebrowser/misc/earlyinit.py b/qutebrowser/misc/earlyinit.py index eaa493cad..21be8ed37 100644 --- a/qutebrowser/misc/earlyinit.py +++ b/qutebrowser/misc/earlyinit.py @@ -337,12 +337,7 @@ def check_libraries(backend): "or Install via pip.", pip="PyYAML"), 'PyQt5.QtSql': - _missing_str("PyQt5.QtSql", - windows="Use the installer by Riverbank computing " - "or the standalone qutebrowser exe. " - "http://www.riverbankcomputing.co.uk/" - "software/pyqt/download5", - pip="PyQt5"), + _missing_str("PyQt5.QtSql") } if backend == 'webengine': modules['PyQt5.QtWebEngineWidgets'] = _missing_str("QtWebEngine", diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index 4db3fb899..35c5359dd 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -209,8 +209,8 @@ class SqlTable(QObject): """Performantly append multiple rows to the table. Args: - rows: A list of lists, where each sub-list is a row. values: A dict with a list of values to insert for each field name. + replace: If true, overwrite rows with a primary key match. """ q = self._insert_query(values, replace) for key, val in values.items(): diff --git a/tests/unit/browser/webkit/test_history.py b/tests/unit/browser/test_history.py similarity index 97% rename from tests/unit/browser/webkit/test_history.py rename to tests/unit/browser/test_history.py index 4b317acab..9496accd2 100644 --- a/tests/unit/browser/webkit/test_history.py +++ b/tests/unit/browser/test_history.py @@ -113,7 +113,8 @@ def test_clear(qtbot, tmpdir, hist, mocker): hist.add_url(QUrl('http://example.com/')) hist.add_url(QUrl('http://www.qutebrowser.org/')) - m = mocker.patch('qutebrowser.browser.history.message.confirm_async') + m = mocker.patch('qutebrowser.browser.history.message.confirm_async', + spec=[]) hist.clear() assert m.called @@ -288,15 +289,19 @@ def test_import_txt_skip(hist, data_tmpdir, line, monkeypatch, stubs): '68891-x http://example.com/bad-flag', '68891 http://.com', ]) -def test_import_txt_invalid(hist, data_tmpdir, line, monkeypatch, stubs): +def test_import_txt_invalid(hist, data_tmpdir, line, monkeypatch, stubs, + caplog): """import_txt should fail on certain lines.""" monkeypatch.setattr(history, 'QTimer', stubs.InstaTimer) histfile = data_tmpdir / 'history' histfile.write(line) - with pytest.raises(Exception): + with caplog.at_level(logging.ERROR): hist.import_txt() + assert any(rec.msg.startswith("Failed to import history:") + for rec in caplog.records) + assert histfile.exists() diff --git a/tests/unit/completion/test_completionmodel.py b/tests/unit/completion/test_completionmodel.py index 3439400cb..bc7f15ab8 100644 --- a/tests/unit/completion/test_completionmodel.py +++ b/tests/unit/completion/test_completionmodel.py @@ -36,7 +36,7 @@ def test_first_last_item(counts): """Test that first() and last() index to the first and last items.""" model = completionmodel.CompletionModel() for c in counts: - cat = mock.Mock() + cat = mock.Mock(spec=['layoutChanged']) cat.rowCount = mock.Mock(return_value=c) model.add_category(cat) nonempty = [i for i, rowCount in enumerate(counts) if rowCount > 0] @@ -70,16 +70,17 @@ def test_count(counts): def test_set_pattern(pat): """Validate the filtering and sorting results of set_pattern.""" model = completionmodel.CompletionModel() - cats = [mock.Mock(spec=['set_pattern', 'layoutChanged'])] * 3 + cats = [mock.Mock(spec=['set_pattern', 'layoutChanged']) for _ in range(3)] for c in cats: - c.set_pattern = mock.Mock() + c.set_pattern = mock.Mock(spec=[]) model.add_category(c) model.set_pattern(pat) - assert all(c.set_pattern.called_with([pat]) for c in cats) + for c in cats: + c.set_pattern.assert_called_with(pat) def test_delete_cur_item(): - func = mock.Mock() + func = mock.Mock(spec=[]) model = completionmodel.CompletionModel() cat = listcategory.ListCategory('', [('foo', 'bar')], delete_func=func) model.add_category(cat) diff --git a/tests/unit/completion/test_completionwidget.py b/tests/unit/completion/test_completionwidget.py index 1625dc60a..cad45b5c1 100644 --- a/tests/unit/completion/test_completionwidget.py +++ b/tests/unit/completion/test_completionwidget.py @@ -83,7 +83,7 @@ def test_set_model(completionview): def test_set_pattern(completionview): model = completionmodel.CompletionModel() - model.set_pattern = mock.Mock() + model.set_pattern = mock.Mock(spec=[]) completionview.set_model(model) completionview.set_pattern('foo') model.set_pattern.assert_called_with('foo') @@ -214,7 +214,7 @@ def test_completion_show(show, rows, quick_complete, completionview, def test_completion_item_del(completionview): """Test that completion_item_del invokes delete_cur_item in the model.""" - func = mock.Mock() + func = mock.Mock(spec=[]) model = completionmodel.CompletionModel() cat = listcategory.ListCategory('', [('foo', 'bar')], delete_func=func) model.add_category(cat) @@ -226,7 +226,7 @@ def test_completion_item_del(completionview): def test_completion_item_del_no_selection(completionview): """Test that completion_item_del with an invalid index.""" - func = mock.Mock() + func = mock.Mock(spec=[]) model = completionmodel.CompletionModel() cat = listcategory.ListCategory('', [('foo',)], delete_func=func) model.add_category(cat) diff --git a/tests/unit/completion/test_listcategory.py b/tests/unit/completion/test_listcategory.py index 377d9abb7..5b6a5eea4 100644 --- a/tests/unit/completion/test_listcategory.py +++ b/tests/unit/completion/test_listcategory.py @@ -1,6 +1,6 @@ # vim: ft=python fileencoding=utf-8 sts=4 sw=4 et: -# Copyright 2015-2016 Florian Bruhin (The Compiler) +# Copyright 2017 Ryan Roden-Corrent (rcorre) # # This file is part of qutebrowser. # diff --git a/tests/unit/completion/test_sqlcategory.py b/tests/unit/completion/test_sqlcategory.py index 951a7d865..10d96f571 100644 --- a/tests/unit/completion/test_sqlcategory.py +++ b/tests/unit/completion/test_sqlcategory.py @@ -135,32 +135,11 @@ def test_select(): utils.validate_model(cat, [('bar', 'baz', 'foo')]) -def test_where(): - table = sql.SqlTable('Foo', ['a', 'b', 'c']) - table.insert({'a': 'foo', 'b': 'bar', 'c': False}) - table.insert({'a': 'baz', 'b': 'biz', 'c': True}) - cat = sqlcategory.SqlCategory('Foo', filter_fields=['a'], where='not c') - cat.set_pattern('') - utils.validate_model(cat, [('foo', 'bar', False)]) - - -def test_group(): - table = sql.SqlTable('Foo', ['a', 'b']) - table.insert({'a': 'foo', 'b': 1}) - table.insert({'a': 'bar', 'b': 3}) - table.insert({'a': 'foo', 'b': 2}) - table.insert({'a': 'bar', 'b': 0}) - cat = sqlcategory.SqlCategory('Foo', filter_fields=['a'], - select='a, max(b)', group_by='a') - cat.set_pattern('') - utils.validate_model(cat, [('bar', 3), ('foo', 2)]) - - def test_delete_cur_item(): table = sql.SqlTable('Foo', ['a', 'b']) table.insert({'a': 'foo', 'b': 1}) table.insert({'a': 'bar', 'b': 2}) - func = unittest.mock.MagicMock() + func = unittest.mock.Mock(spec=[]) cat = sqlcategory.SqlCategory('Foo', filter_fields=['a'], delete_func=func) cat.set_pattern('') cat.delete_cur_item(cat.index(0, 0)) diff --git a/tests/unit/misc/test_lineparser.py b/tests/unit/misc/test_lineparser.py index cd4a64274..0c78035b7 100644 --- a/tests/unit/misc/test_lineparser.py +++ b/tests/unit/misc/test_lineparser.py @@ -58,7 +58,8 @@ class TestBaseLineParser: mocker.patch('builtins.open', mock.mock_open()) with lineparser._open('r'): - with pytest.raises(IOError): + with pytest.raises(IOError, + match="Refusing to double-open LineParser."): with lineparser._open('r'): pass