From 3675662f80bd1463d55855a4c368994b73a14182 Mon Sep 17 00:00:00 2001 From: Gerard Roche Date: Fri, 26 Jan 2024 19:09:27 +0000 Subject: [PATCH] fix: edge-case switchable misses --- CHANGELOG.md | 6 + lib/utils.py | 207 ++++++++++++++---------- tests/lib/utils/test_find_switchable.py | 119 +++++++------- 3 files changed, 189 insertions(+), 143 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d1d16d..d36c7a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All notable changes are documented in this file using the [Keep a CHANGELOG](http://keepachangelog.com/) principles. +## 3.19.2 - Unreleased + +### Fixed + +- Edge-case switchable misses + ## 3.19.1 - 2024-01-23 ### Fixed diff --git a/lib/utils.py b/lib/utils.py index 9ae7733..f2afe4d 100644 --- a/lib/utils.py +++ b/lib/utils.py @@ -342,126 +342,157 @@ def file_encoded_position(self, view): return file + encoded_postion -def find_switchable(view, on_select=None): - # Args: - # view (View) - # on_select (callable) - # - # Returns: - # void - window = view.window() - - if on_select is None: - raise ValueError('a callable is required') - - file = view.file_name() - debug_message('switch: %s', file) +def _get_switchable_lookup_symbols(window, classes: list) -> list: + locations = [] # type: list + for _class in classes: + if _class['class'][-4:] == 'Test': + symbol = _class['class'][:-4] + else: + symbol = _class['class'] + 'Test' - classes = find_php_classes(view, with_namespace=True) - if len(classes) == 0: - return message('could not find a test case or class under test for %s', file) + locations += window.lookup_symbol_in_index(symbol) - debug_message('found %s class in switch: %s', len(classes), classes) + locations = _unique_lookup_symbols(locations) - locations = [] # type: list - for _class in classes: - class_name = _class['class'] + return locations - if class_name[-4:] == 'Test': - symbol = class_name[:-4] - else: - symbol = class_name + 'Test' - symbol_locations = window.lookup_symbol_in_index(symbol) - locations += symbol_locations +def find_switchable(view, on_select) -> None: + window = view.window() - debug_message('found %s possible locations to switch to: %s', len(locations), locations) + classes = find_php_classes(view, with_namespace=True) + debug_message('found %s class(s): %s in %s', len(classes), classes, view.file_name() if view.file_name() else view) - def unique_locations(locations): - locs = [] - seen = set() # type: set - for location in locations: - if location[0] not in seen: - seen.add(location[0]) - locs.append(location) + if not len(classes): + message('Could not find a class in %s', view.file_name() if view.file_name() else view) + return - return locs + lookup_symbols = _get_switchable_lookup_symbols(window, classes) + debug_message('found %s lookup symbol(s): %s', len(lookup_symbols), lookup_symbols) - locations = unique_locations(locations) + if not len(lookup_symbols): + message('Could not find a switchable for %s', view.file_name() if view.file_name() else view) + return - if len(locations) == 0: - if has_test(view): - return message('could not find class under test for %s', file) - else: - return message('could not find test case for %s', file) + def _on_select(index: int) -> None: + if index >= 0: + on_select(Switchable(lookup_symbols[index])) - def _on_select(index): - if index == -1: - return + if len(lookup_symbols) == 1: + _on_select(0) + return - switchable = Switchable(locations[index]) + lookup_symbols, is_exact = _find_switchable_in_lookup_symbols(view.file_name(), lookup_symbols) - if on_select is not None: - on_select(switchable) + debug_message('found %d lookup symbol(s): %s', len(lookup_symbols), lookup_symbols) - if len(locations) == 1: + if is_exact and len(lookup_symbols) == 1: return _on_select(0) - locations, is_exact = refine_switchable_locations(locations=locations, file=file) + window.show_quick_panel(['{}:{}'.format(loc[1], loc[2][0]) for loc in lookup_symbols], _on_select) - debug_message('is_exact=%s', is_exact) - debug_message('locations(%s)=%s', len(locations), locations) - if is_exact and len(locations) == 1: - return _on_select(0) +def _unique_lookup_symbols(locations: list) -> list: + locs = [] + seen = set() # type: set + for location in locations: + if location[0] not in seen: + seen.add(location[0]) + locs.append(location) - window.show_quick_panel(['{}:{}'.format(loc[1], loc[2][0]) for loc in locations], _on_select) + return locs -def refine_switchable_locations(locations, file): - debug_message('refine location') +def _find_switchable_in_lookup_symbols(file, lookup_symbols: list) -> tuple: if not file: - return locations, False + return lookup_symbols, False - debug_message('file=%s', file) - debug_message('locations=%s', locations) + switchable_files, is_test = _get_switchable_files(file) + debug_message('switchable_files=%s', switchable_files) - files = [] - if file.endswith('Test.php'): - file_is_test_case = True - file = file.replace('Test.php', '.php') - files.append(re.sub('(\\/)?[tT]ests\\/([uU]nit\\/)?', '/', file)) - files.append(re.sub('(\\/)?[tT]ests\\/', '/src/', file)) - files.append(re.sub('(\\/)?[tT]ests\\/Unit/', '/app/', file)) - files.append(re.sub('(\\/)?[tT]ests\\/Integration/', '/app/', file)) - else: - file_is_test_case = False - file = file.replace('.php', 'Test.php') - files.append(file) - files.append(re.sub('(\\/)?app\\/', '/tests/', file)) - files.append(re.sub('(\\/)?app\\/', '/tests/Unit/', file)) - files.append(re.sub('(\\/)?app\\/', '/tests/Integration/', file)) - files.append(re.sub('(\\/)?src\\/', '/', file)) - files.append(re.sub('(\\/)?src\\/', '/test/', file)) - - files = list(set(files)) - debug_message('files=%s', files) - - if len(locations) > 1: - common_prefix = os.path.commonprefix([loc[0] for loc in locations]) + # print('->', file) + # for f in switchable_files: + # print('=>', f) + # print('') + # for lookup_symbol in lookup_symbols: + # print('=>', lookup_symbol[0], lookup_symbol[1]) + + for switchable_file in switchable_files: + for lookup_symbol in lookup_symbols: + if lookup_symbol[0] == switchable_file: + return [lookup_symbol], True + + if len(lookup_symbols) > 1: + common_prefix = os.path.commonprefix([loc[0] for loc in lookup_symbols]) if common_prefix != '/': - files = [file.replace(common_prefix, '') for file in files] + switchable_files = [file.replace(common_prefix, '') for file in switchable_files] - for location in locations: + for location in lookup_symbols: loc_file = location[0] - if not file_is_test_case: + if not is_test: loc_file = re.sub('\\/[tT]ests\\/([uU]nit\\/)?', '/', loc_file) - for file in files: + for file in switchable_files: if loc_file.endswith(file): return [location], True - return locations, False + return lookup_symbols, False + + +def _get_switchable_files(file: str) -> tuple: + switchable_files = [] + + if file.endswith('Test.php'): + is_test = True + file = file.replace('Test.php', '.php') + + switchable_file = re.sub('(\\/)?[tT]ests\\/([uU]nit\\/)?', '/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + switchable_file = re.sub('(\\/)?[tT]ests\\/Unit/', '/app/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + switchable_file = re.sub('(\\/)?[tT]ests\\/Integration/', '/app/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + switchable_file = re.sub('(\\/)?[tT]ests\\/', '/src/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + + else: + is_test = False + file = file.replace('.php', 'Test.php') + + # app/ + switchable_file = re.sub('app\\/(?!.*app\\/)', 'tests/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + switchable_file = re.sub('app\\/(?!.*app\\/)', 'tests/Unit/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + switchable_file = re.sub('app\\/(?!.*app\\/)', 'tests/Integration/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + switchable_file = re.sub('app\\/(?!.*app\\/)', '', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + switchable_file = re.sub('app\\/(?!.*app\\/)', 'test/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + + # src/ + switchable_file = re.sub('(\\/)?src\\/', '/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + switchable_file = re.sub('(\\/)?src\\/', '/test/', file, count=1) + if switchable_file not in switchable_files: + switchable_files.append(switchable_file) + + # 1:1 + if file not in switchable_files: + switchable_files.append(file) + + return switchable_files, is_test def put_views_side_by_side(view_a, view_b) -> None: diff --git a/tests/lib/utils/test_find_switchable.py b/tests/lib/utils/test_find_switchable.py index fb5969b..a24d6a2 100644 --- a/tests/lib/utils/test_find_switchable.py +++ b/tests/lib/utils/test_find_switchable.py @@ -17,7 +17,7 @@ from PHPUnitKit.tests import unittest -from PHPUnitKit.lib.utils import refine_switchable_locations +from PHPUnitKit.lib.utils import _find_switchable_in_lookup_symbols from PHPUnitKit.lib.utils import Switchable @@ -44,36 +44,36 @@ def assertLocation(self, file, locations, expected=None): else: expected = expected - self.assertEqual(refine_switchable_locations(locations=locations, file=file), expected) + self.assertEqual(_find_switchable_in_lookup_symbols(file, locations), expected) def assertNotLocation(self, file, location): - self.assertEqual(refine_switchable_locations(locations=[location], file=file), ([location], False)) + self.assertEqual(_find_switchable_in_lookup_symbols(file, [location]), ([location], False)) def test_not_locations(self): - self.assertNotLocation(None, ('/path/to/ATest.php', 'to/ATest.php', (0, 0))) # noqa: E241,E501 - self.assertNotLocation(None, ('/path/to/AClas.php', 'to/AClas.php', (0, 0))) # noqa: E241,E501 - self.assertNotLocation('/path/to/Name.php', ('/foo/bar/NameTest.php', 'bar/NameTest.php', (0, 0))) # noqa: E241,E501 - self.assertNotLocation('/path/to/NameTest.php', ('/foo/bar/Name.php', 'bar/Name.php', (0, 0))) # noqa: E241,E501 + self.assertNotLocation(None, ('/path/to/ATest.php', 'to/ATest.php', (0, 0))) # noqa: E501 + self.assertNotLocation(None, ('/path/to/AClas.php', 'to/AClas.php', (0, 0))) # noqa: E501 + self.assertNotLocation('/path/to/Name.php', ('/foo/bar/NameTest.php', 'bar/NameTest.php', (0, 0))) # noqa: E501 + self.assertNotLocation('/path/to/NameTest.php', ('/foo/bar/Name.php', 'bar/Name.php', (0, 0))) # noqa: E501 def test_test_case(self): - self.assertLocation('/path/to/Name.php', ('/path/to/NameTest.php', 'to/NameTest.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/Name.php', ('/path/to/tests/NameTest.php', 'to/tests/NameTest.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/Name.php', ('/path/to/Tests/NameTest.php', 'to/Tests/NameTest.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/Name.php', ('/path/to/tests/Unit/NameTest.php', 'to/tests/Unit/NameTest.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/Name.php', ('/path/to/tests/unit/NameTest.php', 'to/tests/unit/NameTest.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/Name.php', ('/path/to/Tests/Unit/NameTest.php', 'to/Tests/Unit/NameTest.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/Name.php', ('/path/to/Tests/unit/NameTest.php', 'to/Tests/unit/NameTest.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/a/lib/x/y/tests/NameTest.php', ('/path/to/a/lib/x/y/src/Name.php', 'a/lib/x/y/src/Name.php', (5, 13))) # noqa: E241,E501 + self.assertLocation('/path/to/Name.php', ('/path/to/NameTest.php', 'to/NameTest.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/Name.php', ('/path/to/tests/NameTest.php', 'to/tests/NameTest.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/Name.php', ('/path/to/Tests/NameTest.php', 'to/Tests/NameTest.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/Name.php', ('/path/to/tests/Unit/NameTest.php', 'to/tests/Unit/NameTest.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/Name.php', ('/path/to/tests/unit/NameTest.php', 'to/tests/unit/NameTest.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/Name.php', ('/path/to/Tests/Unit/NameTest.php', 'to/Tests/Unit/NameTest.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/Name.php', ('/path/to/Tests/unit/NameTest.php', 'to/Tests/unit/NameTest.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/a/lib/x/y/tests/NameTest.php', ('/path/to/a/lib/x/y/src/Name.php', 'a/lib/x/y/src/Name.php', (5, 13))) # noqa: E501 def test_class(self): - self.assertLocation('/path/to/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/tests/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/Tests/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/tests/Unit/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/tests/unit/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/Tests/Unit/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/Tests/unit/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E241,E501 - self.assertLocation('/path/to/a/lib/x/y/src/Name.php', ('/path/to/a/lib/x/y/tests/NameTest.php', 'a/lib/x/y/tests/NameTest.php', (5, 13))) # noqa: E241,E501 + self.assertLocation('/path/to/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/tests/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/Tests/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/tests/Unit/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/tests/unit/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/Tests/Unit/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/Tests/unit/NameTest.php', ('/path/to/Name.php', 'to/Name.php', (0, 0))) # noqa: E501 + self.assertLocation('/path/to/a/lib/x/y/src/Name.php', ('/path/to/a/lib/x/y/tests/NameTest.php', 'a/lib/x/y/tests/NameTest.php', (5, 13))) # noqa: E501 def test_test_complicated(self): self.assertLocation( @@ -94,65 +94,74 @@ def test_test_complicated(self): def test_no_match(self): expected = ('/vendor/x/y/tests/a/b/FooTest.php', 'x/y/tests/a/b/FooTest.php', (5, 7)) - actual = refine_switchable_locations(locations=[expected], file='/a/y/z/Foo.php') + actual = _find_switchable_in_lookup_symbols('/a/y/z/Foo.php', [expected]) self.assertEqual(actual, ([expected], False)) def test_match(self): expected = ('/a/b/tests/Unit/app/x/y/ZTest.php', 'b/tests/Unit/app/x/y/ZTest.php', (7, 7)) - actual = refine_switchable_locations(locations=[expected], file='/a/b/app/x/y/Z.php') + actual = _find_switchable_in_lookup_symbols('/a/b/app/x/y/Z.php', [expected]) self.assertEqual(actual, ([expected], True)) def test_match2(self): expected = ('/a/b/x/y/app/X/Y/Name.php', 'app/X/Y/Name.php', (25, 7)) - actual = refine_switchable_locations(locations=[expected], file='/a/b/x/y/tests/Unit/app/X/Y/NameTest.php') + actual = _find_switchable_in_lookup_symbols('/a/b/x/y/tests/Unit/app/X/Y/NameTest.php', [expected]) self.assertEqual(actual, ([expected], True)) def test_match3(self): expected = ('/a/b/c/app/xxx/X.php', 'c/app/xxx/X.php', (60, 13)) - actual = refine_switchable_locations(locations=[ - expected, - ('/a/b/c/app/xxx/X.php', 'c/app/xxx/X.php', (60, 13)), - ('/a/b/c/vendor/Foobar.php', 'c/vendor/Foobar.php', (4, 31)), - ('/a/b/c/vendor/x/y/src/E/X.php', 'c/vendor/x/y/src/E/X.php', (33, 7)), - ('/a/b/c/vendor/x/y/Goutte/X.php', 'c/vendor/x/y/Goutte/X.php', (30, 7)), - ('/a/b/c/vendor/x/y/src/X.php', 'c/vendor/x/y/src/X.php', (25, 7)), - ('/a/b/c/vendor/x/y/src/X.php', 'c/vendor/x/y/src/X.php', (7, 7)), - ('/a/b/c/vendor/x/y/src/B/X.php', 'c/vendor/x/y/src/B/X.php', (9, 7)), - ('/a/b/c/vendor/x/y/src/X.php', 'c/vendor/x/y/src/X.php', (41, 7)), - ('/a/b/c/vendor/x/y/lib/D/X.php', 'c/vendor/x/y/lib/D/X.php', (20, 7)), - ('/a/b/c/vendor/x/y/lib/X.php', 'c/vendor/x/y/lib/X.php', (44, 7)), - ('/a/b/c/vendor/x/y/X.php', 'c/vendor/x/y/X.php', (29, 16)), - ('/a/b/c/vendor/x/y/X.php', 'c/vendor/x/y/X.php', (31, 7))], - file='/a/b/c/tests/Unit/app/xxx/XTest.php') + actual = _find_switchable_in_lookup_symbols( + '/a/b/c/tests/Unit/app/xxx/XTest.php', + [ + expected, + ('/a/b/c/app/xxx/X.php', 'c/app/xxx/X.php', (60, 13)), + ('/a/b/c/vendor/Foobar.php', 'c/vendor/Foobar.php', (4, 31)), + ('/a/b/c/vendor/x/y/src/E/X.php', 'c/vendor/x/y/src/E/X.php', (33, 7)), + ('/a/b/c/vendor/x/y/Goutte/X.php', 'c/vendor/x/y/Goutte/X.php', (30, 7)), + ('/a/b/c/vendor/x/y/src/X.php', 'c/vendor/x/y/src/X.php', (25, 7)), + ('/a/b/c/vendor/x/y/src/X.php', 'c/vendor/x/y/src/X.php', (7, 7)), + ('/a/b/c/vendor/x/y/src/B/X.php', 'c/vendor/x/y/src/B/X.php', (9, 7)), + ('/a/b/c/vendor/x/y/src/X.php', 'c/vendor/x/y/src/X.php', (41, 7)), + ('/a/b/c/vendor/x/y/lib/D/X.php', 'c/vendor/x/y/lib/D/X.php', (20, 7)), + ('/a/b/c/vendor/x/y/lib/X.php', 'c/vendor/x/y/lib/X.php', (44, 7)), + ('/a/b/c/vendor/x/y/X.php', 'c/vendor/x/y/X.php', (29, 16)), + ('/a/b/c/vendor/x/y/X.php', 'c/vendor/x/y/X.php', (31, 7)) + ]) self.assertEqual(actual, ([expected], True)) def test_match_relative_path_matches(self): expected = ('/a/b/c/CTest.php', 'b/c/CTest.php', (3, 5)) - actual = refine_switchable_locations( - locations=[expected, ('/a/x/y/CTest.php', 'x/y/CTest.php', (7, 11))], - file='/a/b/c/C.php' - ) + actual = _find_switchable_in_lookup_symbols( + '/a/b/c/C.php', + [expected, ('/a/x/y/CTest.php', 'x/y/CTest.php', (7, 11))]) self.assertEqual(actual, ([expected], True)) - def test_match_Tests_dir(self): + def test_match_tests_dir(self): expected = ('/a/b/c/Tests/CTest.php', 'b/c/Tests/CTest.php', (3, 5)) - actual = refine_switchable_locations( - locations=[expected, ('/a/x/y/Tests/CTest.php', 'x/y/Tests/CTest.php', (7, 11))], - file='/a/b/c/C.php' - ) + actual = _find_switchable_in_lookup_symbols( + '/a/b/c/C.php', + [expected, ('/a/x/y/Tests/CTest.php', 'x/y/Tests/CTest.php', (7, 11))]) self.assertEqual(actual, ([expected], True)) - def test_match_tests_dir(self): + def test_match_tests_dir2(self): expected = ('/a/b/c/Tests/CTest.php', 'b/c/Tests/CTest.php', (3, 5)) - actual = refine_switchable_locations( - locations=[expected, ('/a/x/y/Tests/CTest.php', 'x/y/Tests/CTest.php', (7, 11))], - file='/a/b/c/C.php' - ) + actual = _find_switchable_in_lookup_symbols( + '/a/b/c/C.php', + [expected, ('/a/x/y/Tests/CTest.php', 'x/y/Tests/CTest.php', (7, 11))]) self.assertEqual(actual, ([expected], True)) + + def test_similar_paths_but_single_match(self): + actual = _find_switchable_in_lookup_symbols('/home/code/x/a/app/app/Models/Account.php', [ + ('/home/code/x/b/other/tests/Integration/Models/AccountTest.php', 'other/tests/Integration/Models/AccountTest.php', (17, 13)), # noqa: E501 + ('/home/code/x/a/app/tests/Integration/Models/AccountTest.php', 'app/tests/Integration/Models/AccountTest.php', (15, 13)) # noqa: E501 + ]) + + self.assertEqual(([ + ('/home/code/x/a/app/tests/Integration/Models/AccountTest.php', 'app/tests/Integration/Models/AccountTest.php', (15, 13)) # noqa: E501 + ], True), actual)