Record suite info in test summary yaml. #949

mhaoli · 2024-12-02T13:07:01Z

Record the suite class name in summary yaml. No information will be added if the test is triggered by suite_runner.run_suite.

To record suite info, this PR adds a new test summary entry type SuiteInfo and record class SuiteInfoRecord.

This change is

xianyuanjia

How would this impact any downstream parsing of the results? Would the new entries be ignored by default?

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @mhaoli and @xpconanfan)

mobly/records.py line 734 at r1 (raw file):

  """A record representing the suite info in test summary."""

  KEY_SUITE_CLASS_NAME = 'Suite Class Name'

Could simplify to Suite Name.

Code quote:

KEY_SUITE_CLASS_NAME

mobly/records.py line 739 at r1 (raw file):

  def __init__(self, suite_class_name):
    self.suite_class_name = suite_class_name
    self.timestamp = time.time()

Would it be better to record a begin and end time instead? So we can report the total execution time of a suite.

Code quote:

    self.timestamp = time.time()

mhaoli

I did some verification and think this CL should be safe. I'd love to know if you suggest other things to be checked.

I ran a mobly_mh_test successfully, consulted Yao about the result parsing logic, did code search to find possible breakage. I only found that a warning will be printed on MH side, I'll ask MH team about how to fix it.

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

mobly/records.py line 734 at r1 (raw file):

Previously, xianyuanjia wrote…

Could simplify to Suite Name.

I'm considering suite name may contain more info than only suite class name. Pinged you to discuss offline.

mobly/records.py line 739 at r1 (raw file):

Previously, xianyuanjia wrote…

Would it be better to record a begin and end time instead? So we can report the total execution time of a suite.

Good idea. DONE.

mobly/records.py

xpconanfan · 2024-12-03T13:11:14Z

Do you really need a new class for a string and two timestamps? Also, why adding a field to the test report when most situations don't involve suite? Could you do this without modifying the records module?

…

On Tue, Dec 3, 2024, 10:05 PM Minghao Li ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mobly/records.py <#949 (comment)>: > - def __init__(self, suite_class_name): - self.suite_class_name = suite_class_name - self.timestamp = time.time() + KEY_SUITE_CLASS = 'Suite Class' The test suite class that inherits from base_suite.BaseSuite. If "Suite Class" is not clear enough, I modified it to "Test Suite Class". — Reply to this email directly, view it on GitHub <#949 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARDNZNL6BSEBKWZLSNXTTT2DWUC7AVCNFSM6AAAAABS3LVKAKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINZVGYZDQMJXGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mhaoli · 2024-12-03T13:32:16Z

Do you really need a new class for a string and two timestamps? Also, why adding a field to the test report when most situations don't involve suite? Could you do this without modifying the records module?

I just thought this is a proper way to record mobly internal info to summary file.

If we want to avoid modifying the records module, I can record the suite info with record type UserData. Modifying.

xpconanfan · 2024-12-04T00:18:46Z

Since the suite info is only created by suite runner, would it make sense to have the suite runner to simply append a section in the results yaml at the end? Also, would it make sense to summarize other info in the suite info? Like suite version, total pass/fail

…

On Tue, Dec 3, 2024, 10:32 PM Minghao Li ***@***.***> wrote: Do you really need a new class for a string and two timestamps? Also, why adding a field to the test report when most situations don't involve suite? Could you do this without modifying the records module? I just thought this is a proper way to record mobly internal info to summary file. If we want to avoid modifying the records module, I can record the suite info with record type UserData. Modifying. — Reply to this email directly, view it on GitHub <#949 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARDNZKMTAZBAFFADW2PC532DWXHNAVCNFSM6AAAAABS3LVKAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJUGU3DQMJXHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…o BaseSuite and record it to test summary

mobly/suite_runner.py

…ite info.

mhaoli

Resolved comments in this thread.

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @xianyuanjia and @xpconanfan)

mobly/records.py line 734 at r1 (raw file):

Previously, mhaoli (Minghao Li) wrote…

I'm considering suite name may contain more info than only suite class name. Pinged you to discuss offline.

Added a field 'Suite Name' which is either the suite class name or a user defined suite name. But I didn't remove field Test Suite Class as I think we need to keep this info.

xianyuanjia

Reviewed 1 of 5 files at r4, 3 of 3 files at r6, all commit messages.
Reviewable status: 4 of 5 files reviewed, 7 unresolved discussions (waiting on @mhaoli and @xpconanfan)

mobly/base_suite.py line 56 at r6 (raw file):

    """The name of this suite.

    By default, use the name of the test class. User can overwrite to return

suite class?

Code quote:

test class

mobly/base_suite.py line 73 at r6 (raw file):

      A dict of suite information. Keys and values must be serializable.
    """
    return {}

There is no implementation here. Is this intentional? If so, please leave a TODO.

Code quote:

return {}

mobly/suite_runner.py line 114 at r6 (raw file):

  _end_time: int | None

  def __init__(self, test_suite_class):

Is there anywhere that indicates clearly that this record only works for suite classes and not suites directly assembled via run_suite?

mobly/suite_runner.py

mhaoli

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

mobly/base_suite.py line 56 at r6 (raw file):

Previously, xianyuanjia wrote…

suite class?

Good catch. DONE.

mobly/base_suite.py line 73 at r6 (raw file):

Previously, xianyuanjia wrote…

There is no implementation here. Is this intentional? If so, please leave a TODO.

This is intentional. This method is for users to override to record customized info to test summary, e.g. suite version. By default, it just returns an empty dict.

Modified docstring to clarify this.

mobly/suite_runner.py line 114 at r6 (raw file):

Previously, xianyuanjia wrote…

Is there anywhere that indicates clearly that this record only works for suite classes and not suites directly assembled via run_suite?

Good point, clarified this point in docstring of this class.

mobly/suite_runner.py

xianyuanjia

Reviewed all commit messages.
Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @xpconanfan)

xpconanfan · 2024-12-30T03:32:17Z

mobly/base_suite.py

+  # Optional interfaces that users can override to record customized suite
+  # information to test summary.
+
+  def get_suite_name(self):


shouldn't this just be part of the suite info?
why should it be a separate getter method?

Test suites can return any custom values in suite info, our infra won't parse it and just write it to summary file.

So I created separate getter methods for fields that our infra needs to parse.

xpconanfan · 2024-12-30T03:32:35Z

mobly/base_suite.py

+    """
+    return self.__class__.__name__
+
+  def get_run_identifier(self):


what is "run identifier"?

This is for key run context that will be displayed as part of the title in our result viewer.

Through this we can include info like the phone brand and phone model.

Record suite class name in summary yaml.

54f9bee

mhaoli added the improvement label Dec 2, 2024

mhaoli added this to the Mobly Release 1.13 milestone Dec 2, 2024

mhaoli requested review from xpconanfan and xianyuanjia December 2, 2024 13:07

mhaoli self-assigned this Dec 2, 2024

Minor fixes and run pyink.

fd0c097

xianyuanjia requested changes Dec 2, 2024

View reviewed changes

Fix comments. Add begin time and end time to suite info record.

3985e78

mhaoli requested a review from xianyuanjia December 3, 2024 12:48

mhaoli commented Dec 3, 2024

View reviewed changes

xpconanfan requested changes Dec 3, 2024

View reviewed changes

mobly/records.py Outdated Show resolved Hide resolved

fix comments

74e4363

Move SuiteInfoRecord class to suite_runner; Add a VERSION attribute t…

efb5d76

…o BaseSuite and record it to test summary

xpconanfan requested changes Dec 4, 2024

View reviewed changes

mobly/suite_runner.py Outdated Show resolved Hide resolved

mobly/suite_runner.py Outdated Show resolved Hide resolved

mhaoli added 2 commits December 4, 2024 20:45

Add a property BaseSuite.suite_info to enable dumping user defined su…

c272a9a

…ite info.

Fix format.

34c1787

mhaoli requested a review from xpconanfan December 10, 2024 07:49

mhaoli added 4 commits December 12, 2024 16:45

Use a dedicate test summary type for suite info record

50003aa

fix format

0684828

Modify API name and docstring

f9bce18

merge master

ce91462

mhaoli changed the title ~~Record suite class name in summary yaml.~~ Record suite info in test summary yaml. Dec 18, 2024

support customized suite name

929cfe8

mhaoli force-pushed the record_suite_class_name branch from c2a8049 to 929cfe8 Compare December 20, 2024 14:31

mhaoli commented Dec 20, 2024

View reviewed changes

xianyuanjia requested changes Dec 23, 2024

View reviewed changes

mobly/suite_runner.py Outdated Show resolved Hide resolved

mhaoli added 2 commits December 24, 2024 16:25

Fix comments; Add an interface get_run_identifier to BaseSuite class.

4b432ed

Polish docstring

47408b0

mhaoli commented Dec 24, 2024

View reviewed changes

mobly/suite_runner.py Outdated Show resolved Hide resolved

mhaoli requested a review from xianyuanjia December 24, 2024 08:47

xianyuanjia approved these changes Dec 26, 2024

View reviewed changes

mhaoli removed the request for review from xpconanfan December 27, 2024 03:35

xpconanfan reviewed Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record suite info in test summary yaml. #949

Record suite info in test summary yaml. #949

mhaoli commented Dec 2, 2024 •

edited by xpconanfan

Loading

xianyuanjia left a comment

mhaoli left a comment

xpconanfan commented Dec 3, 2024 via email

mhaoli commented Dec 3, 2024

xpconanfan commented Dec 4, 2024 via email

mhaoli left a comment

xianyuanjia left a comment

mhaoli left a comment

xianyuanjia left a comment

xpconanfan Dec 30, 2024

mhaoli Dec 30, 2024

xpconanfan Dec 30, 2024

mhaoli Dec 30, 2024

Record suite info in test summary yaml. #949

Are you sure you want to change the base?

Record suite info in test summary yaml. #949

Conversation

mhaoli commented Dec 2, 2024 • edited by xpconanfan Loading

xianyuanjia left a comment

Choose a reason for hiding this comment

mhaoli left a comment

Choose a reason for hiding this comment

xpconanfan commented Dec 3, 2024 via email

mhaoli commented Dec 3, 2024

xpconanfan commented Dec 4, 2024 via email

mhaoli left a comment

Choose a reason for hiding this comment

xianyuanjia left a comment

Choose a reason for hiding this comment

mhaoli left a comment

Choose a reason for hiding this comment

xianyuanjia left a comment

Choose a reason for hiding this comment

xpconanfan Dec 30, 2024

Choose a reason for hiding this comment

mhaoli Dec 30, 2024

Choose a reason for hiding this comment

xpconanfan Dec 30, 2024

Choose a reason for hiding this comment

mhaoli Dec 30, 2024

Choose a reason for hiding this comment

mhaoli commented Dec 2, 2024 •

edited by xpconanfan

Loading