Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separating report_target application in interactive mode #1034

Merged
merged 16 commits into from
Jan 26, 2024

Conversation

M6AI
Copy link
Contributor

@M6AI M6AI commented Dec 19, 2023

Bug / Requirement Description

The report_target decorator is applied in test_context every time the interactive mode accesses it. This results in an overhead during startup, and also runtime.

Solution description

For now we are moving to the runner method with report_target in interactive mode. Longer term we need to make reapplication dependent on the reload.

Checklist:

  • Test
  • Example (both test_plan.py and .rst)
  • Documentation (API)
  • News fragment present for release notes
  • MS info leakage check
  • For new driver: driver index page
  • For new assertion: ui/pdf/std renderers, documentation
  • For new cmdline arg: documentation

@M6AI M6AI requested a review from Pyifan as a code owner December 19, 2023 08:06
getattr(self, "parent", None)
and hasattr(self.parent, "cfg")
and self.parent.cfg.interactive_port is not None
):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied over the condition so that possible refactorization catches both occurrences, there should be better ways to test interactivity.

@Pyifan
Copy link
Contributor

Pyifan commented Jan 3, 2024

I suggest that we address the performance issues more thoroughly, there are 3 of them:

  1. we are calling get_test_context again and again under interactive mode --> suggest that we invalidate self._test_context when reload() is called, then next time test_context is needed, it will re-calculate. I think this only needs a few lines of change (set test._test_context to None in reload()).
  2. testcase_report_target is default on -- this feature is quite heavy on runtime and report size, consider make it default off?
  3. report_target is calculated in get_test_context so in interactive mode, we will do that for all testcases no matter if they are run or not. You have addressed this one by move it to _run_testcases_iter, why not move it to _run_testcase so it is shared by batch & interactive mode?

@M6AI
Copy link
Contributor Author

M6AI commented Jan 3, 2024

I suggest that we address the performance issues more thoroughly, there are 3 of them:

  1. we are calling get_test_context again and again under interactive mode --> suggest that we invalidate self._test_context when reload() is called, then next time test_context is needed, it will re-calculate. I think this only needs a few lines of change (set test._test_context to None in reload()).
  2. testcase_report_target is default on -- this feature is quite heavy on runtime and report size, consider make it default off?
  3. report_target is calculated in get_test_context so in interactive mode, we will do that for all testcases no matter if they are run or not. You have addressed this one by move it to _run_testcases_iter, why not move it to _run_testcase so it is shared by batch & interactive mode?

Except for 2 I agree. The default on came from business requirement, let me reach you offline with that.

def add_created_environment(self, env_uid):
"""Add an environment from the created environment maker instance."""
self.target.add_environment(self._created_environments[env_uid])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All kinds of unused functions, we can restore from history if needed at some point.

continue
else:
new_report[multitest.uid][suite.uid].entries[
case_index
] = suite[case.uid]
except KeyError:
except (KeyError, IndexError):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We checked and this was breaking testcase removal, the reloader would not handle the IndexError that is thrown by lists, we only continued here originally upon KeyError.

@@ -467,7 +454,7 @@ def get_test_context(self):
testcase_instance, None
)
if data is not None:
testcase.__xfail__ = {
testcase.__func__.__xfail__ = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a side effect from report_target, it was turning the bound method into a free function, but here we need to access now the func attribute.

@M6AI M6AI merged commit ce027da into morganstanley:main Jan 26, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants