Support selecting test cases in a class with regex

[xpconanfan]

Now users can enter regex strings with prefix re: in the test_cases field to select with regex. Only full matches apply.

Fixes #887

---

This change is 

[xianyuanjia]

Reviewed 1 of 4 files at r1.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @xpconanfan)

---

mobly/base_test.py line 1039 at r1 (raw file):

      elif test_name in self._generated_test_table:
        test_method = self._generated_test_table[test_name]
      test_methods.append((test_name, test_method))

What happens when a test method is matched by a regex but also specified by an exact string? It seems that the test would be duplicated.

Code quote:

      test_methods.append((test_name, test_method))

---

mobly/base_test.py line 1052 at r1 (raw file):

      if re.fullmatch(test_name_regex, name):
        self._assert_valid_test_name(name)
        matching_name_tuples.append((name, self._generated_test_table[name]))

current_valid_names contains both the predefined and generated test names.

So if you loop through current_valid_names, you can get generated tests, which would fail on getattr.

This is probably something that should also be verified in a unit test.

[xpconanfan]

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @xianyuanjia)

---

mobly/base_test.py line 1039 at r1 (raw file):

Previously, xianyuanjia wrote…

What happens when a test method is matched by a regex but also specified by an exact string? It seems that the test would be duplicated.

We have always allowed the same test case in a class to be selected multiple times actually.

[eric100lin]

Reviewable status: 1 of 4 files reviewed, 9 unresolved discussions (waiting on @xianyuanjia and @xpconanfan)

---

mobly/base_test.py line 1027 at r1 (raw file):

        # process the selector as a regex.
        regex_matching_methods = self._get_regex_matching_test_methods(
            test_name[len(TEST_SELECTOR_REGEX_PREFIX) :], current_valid_names

nit: Consider removeprefix (added in version 3.9) and pass as keyword arguments .

        regex_matching_methods = self._get_regex_matching_test_methods(
            test_name_regex=test_name.removeprefix(TEST_SELECTOR_REGEX_PREFIX),
            current_valid_names=current_valid_names,
        )

Code quote:

test_name[len(TEST_SELECTOR_REGEX_PREFIX) :], current_valid_names

---

mobly/base_test.py line 1034 at r1 (raw file):

      self._assert_valid_test_name(test_name)
      if test_name not in current_valid_names:
        raise Error('%s does not have test method %s.' % (self.TAG, test_name))

nit: consider f-string

Code quote:

'%s does not have test method %s.'

---

mobly/base_test.py line 1047 at r1 (raw file):

    matching_name_tuples = []
    for name in current_valid_names:
      if re.fullmatch(test_name_regex, name):

https://google.github.io/styleguide/pyguide.html#2144-decision
Always use if foo is None: (or is not None) to check for a None value:

if re.fullmatch(test_name_regex, name) is not None:

Code quote:

if re.fullmatch(test_name_regex, name)

---

mobly/base_test.py line 1050 at r1 (raw file):

        matching_name_tuples.append((name, getattr(self, name)))
    for name in self._generated_test_table.keys():
      if re.fullmatch(test_name_regex, name):

ditto

Code quote:

if re.fullmatch(test_name_regex, name)

---

mobly/base_test.py line 1052 at r1 (raw file):

      if re.fullmatch(test_name_regex, name):
        self._assert_valid_test_name(name)
        matching_name_tuples.append((name, self._generated_test_table[name]))

nit: consider taking both key and value

    for name, method in self._generated_test_table.items():
      if re.fullmatch(test_name_regex, name):
        self._assert_valid_test_name(name)
        matching_name_tuples.append((name, method))

Code quote:

    for name in self._generated_test_table.keys():
      if re.fullmatch(test_name_regex, name):
        self._assert_valid_test_name(name)
        matching_name_tuples.append((name, self._generated_test_table[name]))

---

tests/mobly/base_test_test.py line 205 at r1 (raw file):

  def test_cli_test_selection_with_regex(self):
    class MockBaseTest(base_test.BaseTestClass):

Please cover generate_tests case, I found the following error:

  File "mobly/mobly/base_test.py", line 1026, in _get_test_methods
    regex_matching_methods = self._get_regex_matching_test_methods(
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "mobly/mobly/base_test.py", line 1049, in _get_regex_matching_test_methods
    matching_name_tuples.append((name, getattr(self, name)))
                                       ^^^^^^^^^^^^^^^^^^^
AttributeError: 'MockBaseTest' object has no attribute 'test_something_4'

when I add codes like:

      def pre_run(self):
        self.generate_tests(
            test_logic=self.logic,
            name_func=self.name_gen,
            arg_sets=[('something', 4)],
        )

      def name_gen(self, a, b):
        return 'test_%s_%s' % (a, b)

      def logic(self, a, b):
        pass

You might need to check hasattr(self, name) in the first loop.

Code quote:

MockBaseTest

[eric100lin]

Reviewable status: 1 of 4 files reviewed, 10 unresolved discussions (waiting on @xianyuanjia and @xpconanfan)

---

mobly/base_test.py line 1048 at r1 (raw file):

    for name in current_valid_names:
      if re.fullmatch(test_name_regex, name):
        matching_name_tuples.append((name, getattr(self, name)))

I think we should also check self._assert_valid_test_name(name) here,
otherwise I can inject invalid tests like this :)

    class MockBaseTest(base_test.BaseTestClass):

        # ...

      def get_existing_test_names(self):
        return [
            'logic2',
        ]

      def logic2(self):
        pass

    bt_cls = MockBaseTest(self.mock_test_cls_configs)
    bt_cls.run(test_names=['re:logic2'])

Code quote:

matching_name_tuples.append((name, getattr(self, name)))

[xpconanfan]

Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @eric100lin, @ko1in1u, and @xianyuanjia)

---

mobly/base_test.py line 1045 at r1 (raw file):

Previously, ko1in1u (Kolin Lu) wrote…

Ah, got it. At first glance, it looks like a tuple type from its name.

Done.

---

mobly/base_test.py line 1047 at r1 (raw file):

Previously, eric100lin (Eric Lin (Tzu Hsiang Lin)) wrote…

https://google.github.io/styleguide/pyguide.html#2144-decision
Always use if foo is None: (or is not None) to check for a None value:

if re.fullmatch(test_name_regex, name) is not None:

Done.

[xpconanfan]

Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @eric100lin, @ko1in1u, and @xianyuanjia)

---

mobly/base_test.py line 1048 at r1 (raw file):

Previously, eric100lin (Eric Lin (Tzu Hsiang Lin)) wrote…

I think we should also check self._assert_valid_test_name(name) here,
otherwise I can inject invalid tests like this :)

    class MockBaseTest(base_test.BaseTestClass):

        # ...

      def get_existing_test_names(self):
        return [
            'logic2',
        ]

      def logic2(self):
        pass

    bt_cls = MockBaseTest(self.mock_test_cls_configs)
    bt_cls.run(test_names=['re:logic2'])

well, if you consider the case of overriding get_existing_test_names, then you can say the _assert_valid_test_name can also be overriden... and we have to draw the line somewhere right..

---

mobly/base_test.py line 1050 at r1 (raw file):

Previously, eric100lin (Eric Lin (Tzu Hsiang Lin)) wrote…

ditto

Done.

---

tests/mobly/base_test_test.py line 205 at r1 (raw file):

Previously, eric100lin (Eric Lin (Tzu Hsiang Lin)) wrote…

Please cover generate_tests case, I found the following error:

  File "mobly/mobly/base_test.py", line 1026, in _get_test_methods
    regex_matching_methods = self._get_regex_matching_test_methods(
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "mobly/mobly/base_test.py", line 1049, in _get_regex_matching_test_methods
    matching_name_tuples.append((name, getattr(self, name)))
                                       ^^^^^^^^^^^^^^^^^^^
AttributeError: 'MockBaseTest' object has no attribute 'test_something_4'

when I add codes like:

      def pre_run(self):
        self.generate_tests(
            test_logic=self.logic,
            name_func=self.name_gen,
            arg_sets=[('something', 4)],
        )

      def name_gen(self, a, b):
        return 'test_%s_%s' % (a, b)

      def logic(self, a, b):
        pass

You might need to check hasattr(self, name) in the first loop.

Done.

[eric100lin]

Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @ko1in1u, @xianyuanjia, and @xpconanfan)

---

mobly/base_test.py line 1048 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

well, if you consider the case of overriding get_existing_test_names, then you can say the _assert_valid_test_name can also be overriden... and we have to draw the line somewhere right..

But get_existing_test_names is not start with _ prefix, it means open for users to overwrite it.
It's nice to have that check.

[eric100lin]

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @ko1in1u, @xianyuanjia, and @xpconanfan)

---

mobly/base_test.py line 1048 at r1 (raw file):

Previously, eric100lin (Eric Lin (Tzu Hsiang Lin)) wrote…

But get_existing_test_names is not start with _ prefix, it means open for users to overwrite it.
It's nice to have that check.

I see you update the logic for this path, marked as resolved.

[eric100lin]

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ko1in1u, @xianyuanjia, and @xpconanfan)

[xpconanfan]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ko1in1u and @xianyuanjia)

---

mobly/base_test.py line 1052 at r1 (raw file):

Previously, xianyuanjia wrote…

current_valid_names contains both the predefined and generated test names.

So if you loop through current_valid_names, you can get generated tests, which would fail on getattr.

This is probably something that should also be verified in a unit test.

Done.

[xianyuanjia]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ko1in1u)

[xianyuanjia]

Dismissed @ko1in1u from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @xpconanfan)