Merge branch 'fix-report-ordering' into 'master'

Sort reports by line and column

*Description of changes*

The reports have been incorrectly sorted. This fixes the sorting order to use the line number and then column. It also adds a test to verify that the sorting algorithm works.

*Related to:*  #196

See merge request !100

src/flake8/checker.py

@@ -306,7 +306,7 @@ class Manager(object):
         """
         results_reported = results_found = 0
         for checker in self.checkers:
-            results = sorted(checker.results, key=lambda tup: (tup[2], tup[3]))
+            results = sorted(checker.results, key=lambda tup: (tup[1], tup[2]))
             results_reported += self._handle_results(checker.display_name,
                                                      results)
             results_found += len(results)

tests/integration/test_checker.py

@@ -76,3 +76,71 @@ def test_handle_file_plugins(plugin_target):
                                    line_number=EXPECTED_REPORT[0],
                                    column=EXPECTED_REPORT[1],
                                    text=EXPECTED_REPORT[2])
+
+
+PLACEHOLDER_CODE = 'some_line = "of" * code'
+
+
+@pytest.mark.parametrize('results, expected_order', [
+    # No entries should be added
+    ([], []),
+    # Results are correctly ordered
+    ([('A101', 1, 1, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 2, 1, 'placeholder error', PLACEHOLDER_CODE)], [0, 1]),
+    # Reversed order of lines
+    ([('A101', 2, 1, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 1, 1, 'placeholder error', PLACEHOLDER_CODE)], [1, 0]),
+    # Columns are not ordered correctly (when reports are ordered correctly)
+    ([('A101', 1, 2, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 1, 1, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 2, 1, 'placeholder error', PLACEHOLDER_CODE)], [1, 0, 2]),
+    ([('A101', 2, 1, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 1, 1, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 1, 2, 'placeholder error', PLACEHOLDER_CODE)], [1, 2, 0]),
+    ([('A101', 1, 2, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 2, 2, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 2, 1, 'placeholder error', PLACEHOLDER_CODE)], [0, 2, 1]),
+    ([('A101', 1, 3, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 2, 2, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 3, 1, 'placeholder error', PLACEHOLDER_CODE)], [0, 1, 2]),
+    ([('A101', 1, 1, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 1, 3, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 2, 2, 'placeholder error', PLACEHOLDER_CODE)], [0, 1, 2]),
+    # Previously sort column and message (so reversed) (see bug 196)
+    ([('A101', 1, 1, 'placeholder error', PLACEHOLDER_CODE),
+      ('A101', 2, 1, 'charlie error', PLACEHOLDER_CODE)], [0, 1]),
+])
+def test_report_order(results, expected_order):
+    """
+    Test in which order the results will be reported.
+
+    It gets a list of reports from the file checkers and verifies that the
+    result will be ordered independent from the original report.
+    """
+    def count_side_effect(name, sorted_results):
+        """Side effect for the result handler to tell all are reported."""
+        return len(sorted_results)
+
+    # To simplify the parameters (and prevent copy & pasting) reuse report
+    # tuples to create the expected result lists from the indexes
+    expected_results = [results[index] for index in expected_order]
+
+    file_checker = mock.Mock(spec=['results', 'display_name'])
+    file_checker.results = results
+    file_checker.display_name = 'placeholder'
+
+    style_guide = mock.Mock(spec=['options'])
+
+    # Create a placeholder manager without arguments or plugins
+    # Just add one custom file checker which just provides the results
+    manager = checker.Manager(style_guide, [], [])
+    manager.checkers = [file_checker]
+
+    # _handle_results is the first place which gets the sorted result
+    # Should something non-private be mocked instead?
+    handler = mock.Mock()
+    handler.side_effect = count_side_effect
+    manager._handle_results = handler
+
+    assert manager.report() == (len(results), len(results))
+    handler.assert_called_once_with('placeholder', expected_results)