Merge pull request #237 from rgalonso/fix/identifier-case-sensitivity…

…-bugs

fix identifier case sensitivity bugs

Issue.py

@@ -2,7 +2,7 @@ class Issue(object):
     """Basic Issue model for collecting the necessary info to send to GitHub."""
 
     def __init__(self, title, labels, assignees, milestone, body, hunk, file_name,
-                 start_line, num_lines, markdown_language, status, identifier, ref, issue_url, issue_number):
+                 start_line, num_lines, markdown_language, status, identifier, identifier_actual, ref, issue_url, issue_number, start_line_within_hunk=1):
         self.title = title
         self.labels = labels
         self.assignees = assignees
@@ -15,7 +15,15 @@ def __init__(self, title, labels, assignees, milestone, body, hunk, file_name,
         self.markdown_language = markdown_language
         self.status = status
         self.identifier = identifier
+        self.identifier_actual = identifier_actual
         self.ref = ref
         self.issue_url = issue_url
         self.issue_number = issue_number
+        self.start_line_within_hunk = start_line_within_hunk
 
+    def __str__(self):
+        selflist = []
+        for key in [x for x in vars(self).keys() if x not in ("hunk")]:
+            selflist.append(f'"{key}": "{getattr(self, key)}"')
+        selflist.append((f'"hunk": "{self.hunk}"'))
+        return '\n'.join(selflist)

tests/test_edit_py.diff

@@ -0,0 +1,15 @@
+diff --git a/example_file.py b/example_file.py
+index 6b0c6cf..b37e70a 100644
+@@ -1,3 +1,9 @@
++def imaginative_string():
++    return 'figment of my imagination'
++
+ def hello_world():
+-    # TODO: Come up with a more imaginative greeting
+-    print('Hello world')
+\ No newline at end of file
++    print(f'Hello {imaginative_string()}')
++
++    # TODO: Do more stuff
++    #  This function should probably do something more interesting
++    #  labels: help wanted

tests/test_new.diff

@@ -53,7 +53,7 @@ new file mode 100644
 index 0000000..525e25d
 --- /dev/null
 +++ b/example_file.py
-@@ -0,0 +1,23 @@
+@@ -0,0 +1,36 @@
 +def hello_world():
 +    # TODO: Come up with a more imaginative greeting
 +    print('Hello world')
@@ -77,6 +77,19 @@ index 0000000..525e25d
 +    kept together as one.
 +    '''
 +    pass
++
++"""
++The docstring documentation of MyClass
++
++Using a docstring in order to generate autodocumentation
++"""
++def SuffixAdder:
++  def __init__(self, base_str):
++    self.base_str=base_str
++
++  def add_suffix(self, suffix):
++    # add the suffix after the base string
++    return self.base_str + suffix
 \ No newline at end of file
 diff --git a/example.hs b/example.hs
 new file mode 100644

tests/test_new_py.diff

@@ -0,0 +1,10 @@
+diff --git a/example_file.py b/example_file.py
+new file mode 100644
+index 0000000..525e25d
+--- /dev/null
++++ b/example_file.py
+@@ -0,0 +1,3 @@
++def hello_world():
++    # TODO: Come up with a more imaginative greeting
++    print('Hello world')
+\ No newline at end of file

tests/test_process_diff.py

@@ -11,66 +11,113 @@
 
 
 class IssueUrlInsertionTest(unittest.TestCase):
+    _original_addSubTest = None
+    num_subtest_failures = 0
     orig_cwd = None
     tempdir = None
     diff_file = None
     parser = None
 
-    def _setUp(self, diff_file):
+    def _setUp(self, diff_files):
+        # reset counter
+        self.num_subtest_failures = 0
+
         # get current working directory
         self.orig_cwd = os.getcwd()
 
         # Create temporary directory to hold simulated filesystem.
         self.tempdir = tempfile.TemporaryDirectory()
 
-        # run patch against the diff file to generate the simulated filesystem
-        subprocess.run(['patch', '-d', self.tempdir.name,
-                        '-i', f'{os.getcwd()}/tests/{diff_file}'],
-                        stdout=subprocess.DEVNULL,
-                        stderr=subprocess.DEVNULL,
-                        check=True)
+        for diff_file in diff_files:
+            # run patch against the diff file to generate/update the simulated filesystem
+            subprocess.run(['patch', '-d', self.tempdir.name,
+                            '-i', f'{os.getcwd()}/tests/{diff_file}'],
+                            stdout=subprocess.DEVNULL,
+                            stderr=subprocess.DEVNULL,
+                            check=True)
 
-        self.diff_file = open(f'tests/{diff_file}', 'r')
+        self.diff_file = open(f'tests/{diff_files[-1]}', 'r')
         self.parser = TodoParser()
         with open('syntax.json', 'r') as syntax_json:
             self.parser.syntax_dict = json.load(syntax_json)
 
         # change to the simulated filesystem directory
         os.chdir(self.tempdir.name)
 
-    def _standardTest(self, expected_count):
+    def _standardTest(self, expected_count, output_log_on_failure=True):
         # create object to hold output
         output = io.StringIO()
         # process the diffs
         self.raw_issues = process_diff(diff=self.diff_file, insert_issue_urls=True, parser=self.parser, output=output)
+        # store the log for later processing
+        self.output_log = output.getvalue()
         # make sure the number of issue URL comments inserted is as expected
         self.assertEqual(output.getvalue().count('Issue URL successfully inserted'),
                          expected_count,
-                         msg='\nProcessing log\n--------------\n'+output.getvalue())
+                         msg=(
+                             '\nProcessing log\n--------------\n'+output.getvalue()
+                                if output_log_on_failure else None))
+
+    def _addSubTest(self, test, subtest, outcome):
+        if outcome:
+            self.num_subtest_failures+=1
+        if self._original_addSubTest:
+            self._original_addSubTest(test, subtest, outcome)
+
+    def run(self, result=None):
+        if result and getattr(result, "addSubTest", None):
+            self._original_addSubTest = result.addSubTest
+            result.addSubTest = self._addSubTest
+
+        super().run(result)
 
     # this test can take a while and, as far as TodoParser is concerned,
     # redundant with the tests of test_todo_parser, so enable the means
     # to skip it if desired
     @unittest.skipIf(os.getenv('SKIP_PROCESS_DIFF_TEST', 'false') == 'true',
                      "Skipping because 'SKIP_PROCESS_DIFF_TEST' is 'true'")
     def test_url_insertion(self):
-        self._setUp('test_new.diff')
+        self._setUp(['test_new.diff'])
         self._standardTest(80)
 
-    # There is a known bug related to this issue, so until it's resolved
-    # this is an expected failure.
-    # See #225 and #224
+    # See GitHub issue #236
     @unittest.expectedFailure
+    def test_line_numbering_with_deletions(self):
+        self._setUp(['test_new_py.diff', 'test_edit_py.diff'])
+        with self.subTest("Issue URL insertion"):
+            # was issue URL successfully inserted?
+            self._standardTest(1, False)
+        with self.subTest("Issue insertion line numbering"):
+            # make sure the log reports having inserted the issue based on the
+            # correct line numbering of the updated file
+            self.assertIn("Processing issue 1 of 2: 'Do more stuff' @ example_file.py:7",
+                          self.output_log)
+        with self.subTest("Issue deletion line numbering"):
+            # make sure the log reports having closed the issue based on the
+            # correct line numbering of the old (not the updated!) file
+            self.assertIn("Processing issue 2 of 2: 'Come up with a more imaginative greeting' @ example_file.py:2",
+                          self.output_log)
+
+        if self.num_subtest_failures > 0:
+            self.fail(
+                '\n'.join([
+                '',
+                'One or more subtests have failed',
+                'Processing log',
+                '--------------',
+                ''])+
+                self.output_log)
+
     def test_same_title_in_same_file(self):
-        self._setUp('test_same_title_in_same_file.diff')
+        self._setUp(['test_same_title_in_same_file.diff'])
         self._standardTest(5)
 
     # There is a known bug related to this issue, so until it's resolved
     # this is an expected failure.
     # See #229
     @unittest.expectedFailure
     def test_comment_suffix_after_source_line(self):
-        self._setUp('test_comment_suffix_after_source_line.diff')
+        self._setUp(['test_comment_suffix_after_source_line.diff'])
         self._standardTest(1)
         # get details about the issue and source file
         issue = self.raw_issues[0]

tests/test_todo_parser.py

@@ -12,6 +12,22 @@ def count_issues_for_file_type(raw_issues, file_type):
             num_issues += 1
     return num_issues
 
+def get_issues_for_fields(raw_issues, fields):
+    matching_issues = []
+    for issue in raw_issues:
+        for key in fields.keys():
+            if getattr(issue, key) != fields.get(key):
+                break
+        else:
+            matching_issues.append(issue)
+    return matching_issues
+
+def print_unexpected_issues(unexpected_issues):
+    return '\n'.join([
+           '',
+           'Unexpected issues:',
+           '\n=========================\n'.join(map(str, unexpected_issues))])
+
 
 class NewIssueTest(unittest.TestCase):
     # Check for newly added TODOs across the files specified.
@@ -112,6 +128,70 @@ def test_liquid_issues(self):
     def test_lua_issues(self):
         self.assertEqual(count_issues_for_file_type(self.raw_issues, 'lua'), 2)
 
+
+class CustomOptionsTest(unittest.TestCase):
+    def setUp(self):
+        parser = TodoParser(options={"identifiers":
+                                     [{"name": "FIX", "labels": []},
+                                      {"name": "TODO", "labels": []}]})
+        self.raw_issues = []
+        with open('syntax.json', 'r') as syntax_json:
+            parser.syntax_dict = json.load(syntax_json)
+        with open('tests/test_new.diff', 'r') as diff_file:
+            self.raw_issues.extend(parser.parse(diff_file))
+
+    def test_exact_identifier_match(self):
+        """
+        Verify that issues are only created when there's an exact identifier match
+
+        Other than case-insensitivity, an issue should only be matched if the
+        identifier is exactly within the list of identifiers. For instances, if
+        "FIX" is an identifier, it should NOT accidentaly match comments with
+        the words "suffix" or "prefix".
+        """
+        matching_issues = get_issues_for_fields(self.raw_issues,
+                                                {
+                                                    "file_name": "example_file.py",
+                                                    "identifier": "FIX"
+                                                })
+        self.assertEqual(len(matching_issues), 0,
+                         msg=print_unexpected_issues(matching_issues))
+
+    # See GitHub issue #235
+    @unittest.expectedFailure
+    def test_multiple_identifiers(self):
+        """
+        Verify that issues by matching the first identifier on the line
+
+        Issues should be identified such that the priority is where the identifier
+        is found within the comment line, which is not necessarily the order they're
+        specified in the identifier dictionary. For instance, if the dictionary is
+            [{"name": "FIX", "labels": []},
+             {"name": "TODO", "labels": []}]})
+        then a comment line such as
+            # TODO: Fix this
+        should match because of the "TODO", not because of the "Fix". This is not
+        a trivial difference. If it matches for the "TODO", then the title will be
+        "Fix this", but if it matches for the "Fix", then the title will erroneously
+        be just "this".
+        """
+        matching_issues = get_issues_for_fields(self.raw_issues,
+                                                {
+                                                    "file_name": "init.lua",
+                                                    "identifier": "FIX"
+                                                })
+        self.assertEqual(len(matching_issues), 0,
+                         msg=print_unexpected_issues(matching_issues))
+
+        matching_issues = get_issues_for_fields(self.raw_issues,
+                                                {
+                                                    "file_name": "init.lua",
+                                                    "identifier": "TODO"
+                                                })
+        self.assertEqual(len(matching_issues), 2,
+                         msg=print_unexpected_issues(matching_issues))
+
+
 class ClosedIssueTest(unittest.TestCase):
     # Check for removed TODOs across the files specified.
     def setUp(self):