-
Notifications
You must be signed in to change notification settings - Fork 14
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
Refactor parse test result logics #720
Conversation
7d24e90
to
6727695
Compare
6727695
to
e31c077
Compare
@ono-max Not rush, What do you think my change?? |
launchable/test_runners/cucumber.py
Outdated
@@ -256,16 +257,16 @@ def parse_func(self, report_file: str) -> Generator[CaseEventType, None, None]: | |||
{"type": "testcase", "name": test_case}, | |||
] | |||
|
|||
for step in test_case_info.steps: | |||
for step in test_case_info.steps(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt we can defined this logic as a method
launchable/test_runners/cucumber.py
Outdated
for d in data: | ||
file_name = d.get("uri", "") | ||
class_name = d.get("name", "") | ||
# cucumber can define multiple Background steps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? I don't think we can have multiple Background in this context.
You can only have one set of Background steps per Feature or Rule. If you need different Background steps for different scenarios, consider breaking up your set of scenarios into more Rules or more Features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I have to write “multiple steps of a Background “
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I still don't understand what you said. In which case do we need to unify Background durations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unified_backgrounds
variable is assigned None
at https://github.com/launchableinc/cli/pull/720/files#diff-959e6ab7da948bb1bcd21d094e6921c6b5d9027f75db90e13901c16753f689d5R243.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thus, I believe that there is no case that we need to unify Background durations. Can you please tell me if I am missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood what you wanted to say. I'll update it, please wait my fixing
super().__init__(statuses=statuses, duration_nano_sec=duration_nano_sec, error_message=stderr) | ||
|
||
|
||
def _parse_hook_from_element(element: Dict[str, List]) -> TestCaseHookInfo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be too magical, so it's up to you. However, we can write like this.
def _parse_hook_from_element(element: Dict[str, List]) -> TestCaseHookInfo:
duration_nano_sec = 0
statuses = []
stderr = []
def _parse_step_result(step: Dict[str, Dict]):
result = step.get("result", None)
if result:
nonlocal duration_nano_sec
duration_nano_sec += result.get("duration", 0)
statuses.append(result.get("status"))
if result.get("error_message", None):
stderr.append(result["error_message"])
for step in element.get("before", []):
_parse_step_result(step)
for step in element.get("after", []):
_parse_step_result(step)
return TestCaseHookInfo(
duration_nano_sec=duration_nano_sec,
statuses=statuses,
stderr=stderr
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I apply it
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent refactoring! Thanks!
Current parse logic is a little difficult to read and understand.
So, renamed and defined some methods for easy to read and understand
I didn't edit any test codes, so the result might not changed.