launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/hasquerycount-legible-mismatch into lp:launchpad.

Commit message:
Make HasQueryCount mismatches legible, using formatting similar to LP_DEBUG_SQL(_EXTRA) output.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/hasquerycount-legible-mismatch/+merge/333244

The rendering of HasQueryCount mismatches has annoyed me for years: trying to read reprs of enormous multi-line backtraces is not as much fun as a fun thing.  This is actually more or less readable now (length notwithstanding), and is colourised sensibly by bin/test -c.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/hasquerycount-legible-mismatch into lp:launchpad.

=== modified file 'lib/lp/testing/matchers.py'
--- lib/lp/testing/matchers.py	2016-12-02 12:52:56 +0000
+++ lib/lp/testing/matchers.py	2017-11-06 00:25:41 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -207,8 +207,17 @@
 
     @staticmethod
     def _getQueryDetails(collector):
-        result = [unicode(query).encode('utf8') for query in collector.queries]
-        return Content(UTF8_TEXT, lambda: ['\n'.join(result)])
+        result = []
+        for query in collector.queries:
+            start, stop, dbname, statement, backtrace = query
+            result.append(u'%d-%d@%s %s' % (
+                start, stop, dbname, statement.rstrip()))
+            result.append(u'-' * 70)
+            if backtrace is not None:
+                result.append(backtrace.rstrip())
+                result.append(u'.' * 70)
+        result = [item.encode('UTF-8') for item in result]
+        return Content(UTF8_TEXT, lambda: [b'\n'.join(result)])
 
     def get_details(self):
         details = {}

=== modified file 'lib/lp/testing/tests/test_matchers.py'
--- lib/lp/testing/tests/test_matchers.py	2015-10-13 16:58:20 +0000
+++ lib/lp/testing/tests/test_matchers.py	2017-11-06 00:25:41 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -218,17 +218,59 @@
         matcher = HasQueryCount(LessThan(2))
         collector = RequestTimelineCollector()
         collector.count = 2
-        collector.queries = [("foo", "bar"), ("baaz", "quux")]
-        mismatch = matcher.match(collector)
-        self.assertThat(mismatch, Not(Is(None)))
-        details = mismatch.get_details()
-        lines = []
-        for name, content in details.items():
-            self.assertEqual("queries", name)
-            self.assertEqual("text", content.content_type.type)
-            lines.append(''.join(content.iter_text()))
-        self.assertEqual(["('foo', 'bar')\n('baaz', 'quux')"],
-            lines)
+        collector.queries = [
+            (0, 1, "SQL-main-slave", "SELECT 1 FROM Person", None),
+            (2, 3, "SQL-main-slave", "SELECT 1 FROM Product", None),
+            ]
+        mismatch = matcher.match(collector)
+        self.assertThat(mismatch, Not(Is(None)))
+        details = mismatch.get_details()
+        lines = []
+        for name, content in details.items():
+            self.assertEqual("queries", name)
+            self.assertEqual("text", content.content_type.type)
+            lines.append(''.join(content.iter_text()))
+        separator = "-" * 70
+        expected_lines = [
+            "0-1@SQL-main-slave SELECT 1 FROM Person\n" + separator + "\n" +
+            "2-3@SQL-main-slave SELECT 1 FROM Product\n" + separator,
+            ]
+        self.assertEqual(expected_lines, lines)
+        self.assertEqual(
+            "queries do not match: %s" % (LessThan(2).match(2).describe(),),
+            mismatch.describe())
+
+    def test_with_backtrace(self):
+        matcher = HasQueryCount(LessThan(2))
+        collector = RequestTimelineCollector()
+        collector.count = 2
+        collector.queries = [
+            (0, 1, "SQL-main-slave", "SELECT 1 FROM Person",
+             '  File "example", line 2, in <module>\n'
+             '    Store.of(Person).one()\n'),
+            (2, 3, "SQL-main-slave", "SELECT 1 FROM Product",
+             '  File "example", line 3, in <module>\n'
+             '    Store.of(Product).one()\n'),
+            ]
+        mismatch = matcher.match(collector)
+        self.assertThat(mismatch, Not(Is(None)))
+        details = mismatch.get_details()
+        lines = []
+        for name, content in details.items():
+            self.assertEqual("queries", name)
+            self.assertEqual("text", content.content_type.type)
+            lines.append(''.join(content.iter_text()))
+        separator = "-" * 70
+        backtrace_separator = "." * 70
+        expected_lines = [
+            '0-1@SQL-main-slave SELECT 1 FROM Person\n' + separator + '\n' +
+            '  File "example", line 2, in <module>\n' +
+            '    Store.of(Person).one()\n' + backtrace_separator + '\n' +
+            '2-3@SQL-main-slave SELECT 1 FROM Product\n' + separator + '\n' +
+            '  File "example", line 3, in <module>\n' +
+            '    Store.of(Product).one()\n' + backtrace_separator,
+            ]
+        self.assertEqual(expected_lines, lines)
         self.assertEqual(
             "queries do not match: %s" % (LessThan(2).match(2).describe(),),
             mismatch.describe())
@@ -236,10 +278,17 @@
     def test_byEquality(self):
         old_collector = RequestTimelineCollector()
         old_collector.count = 2
-        old_collector.queries = [("a", "1"), ("b", "2")]
+        old_collector.queries = [
+            (0, 1, "SQL-main-slave", "SELECT 1 FROM Person", None),
+            (2, 3, "SQL-main-slave", "SELECT 1 FROM Product", None),
+            ]
         new_collector = RequestTimelineCollector()
         new_collector.count = 3
-        new_collector.queries = [("a", "1"), ("b", "2"), ("c", "3")]
+        new_collector.queries = [
+            (0, 1, "SQL-main-slave", "SELECT 1 FROM Person", None),
+            (2, 3, "SQL-main-slave", "SELECT 1 FROM Product", None),
+            (4, 5, "SQL-main-slave", "SELECT 1 FROM Distribution", None),
+            ]
         matcher = HasQueryCount.byEquality(old_collector)
         mismatch = matcher.match(new_collector)
         self.assertThat(mismatch, Not(Is(None)))
@@ -251,8 +300,18 @@
         old_lines.append("".join(details["other_queries"].iter_text()))
         self.assertEqual("text", details["queries"].content_type.type)
         new_lines.append("".join(details["queries"].iter_text()))
-        self.assertEqual(["('a', '1')\n('b', '2')"], old_lines)
-        self.assertEqual(["('a', '1')\n('b', '2')\n('c', '3')"], new_lines)
+        separator = "-" * 70
+        expected_old_lines = [
+            "0-1@SQL-main-slave SELECT 1 FROM Person\n" + separator + "\n" +
+            "2-3@SQL-main-slave SELECT 1 FROM Product\n" + separator,
+            ]
+        expected_new_lines = [
+            "0-1@SQL-main-slave SELECT 1 FROM Person\n" + separator + "\n" +
+            "2-3@SQL-main-slave SELECT 1 FROM Product\n" + separator + "\n" +
+            "4-5@SQL-main-slave SELECT 1 FROM Distribution\n" + separator,
+            ]
+        self.assertEqual(expected_old_lines, old_lines)
+        self.assertEqual(expected_new_lines, new_lines)
         self.assertEqual(
             "queries do not match: %s" % (Equals(2).match(3).describe(),),
             mismatch.describe())