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

tests: twister: using statemachine for status #80623

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hakehuang
Copy link
Collaborator

@hakehuang hakehuang commented Oct 30, 2024

using statuemachine in status change.
in stastus.py we can generated the statuemachine
by get_graph

The purposes of this PR:

  1. use statuemachine transition to verify the status change in twister runtime. if there is a violate, TransitionNotAllowed exception will be triggered, and call context will be print.
  2. the transition is defined in the statues.py with class TwisterStatusMachine_inner.cycles, you can generate the status.png by run python3 ./statues.py
  3. with this, we can see and control the status flow to avoid mis-use.
  4. enable call stack print if a exception is triggered, in error.py
  5. fix some status error found by this.

@hakehuang
Copy link
Collaborator Author

hakehuang commented Oct 30, 2024

state

with this, we can observe the support test state transition as above

@hakehuang hakehuang marked this pull request as draft October 30, 2024 11:52
@hakehuang hakehuang force-pushed the order_test_status branch 7 times, most recently from 673ece9 to 7c6608d Compare October 31, 2024 08:10
@golowanow
Copy link
Member

with this, we can observe the support test state transition as above

is there already differentiation on status transitions for Harness, TestCase, TestSuite, and TestInstance ?

@hakehuang
Copy link
Collaborator Author

hakehuang commented Oct 31, 2024

is there already differentiation on status transitions for Harness, TestCase, TestSuite, and TestInstance ?

No, but I need clean the ci failures. BTW do you happen to know why this change does not get execute?
https://github.com/zephyrproject-rtos/zephyr/actions/runs/11608284181/workflow?pr=80623#L103

@nashif
Copy link
Member

nashif commented Oct 31, 2024

is there already differentiation on status transitions for Harness, TestCase, TestSuite, and TestInstance ?

No, but I need clean the ci failures. BTW do you happen to know why this change does not get execute? zephyrproject-rtos/zephyr/actions/runs/11608284181/workflow?pr=80623#L103

because we are running in pull_request_target mode.

@hakehuang
Copy link
Collaborator Author

because we are running in pull_request_target mode.

then how can I change this? @nashif

@hakehuang hakehuang force-pushed the order_test_status branch 5 times, most recently from c02927f to 43ea148 Compare November 1, 2024 04:33
@hakehuang hakehuang force-pushed the order_test_status branch 4 times, most recently from eddffa7 to 01598ad Compare November 14, 2024 04:31
@PerMac
Copy link
Member

PerMac commented Nov 14, 2024

@hakehuang to elaborate: pull_request_target means that the source is taken from the target pranch, not the PR. So your change has to be already in the tree to take place, which afaik doesn't allow to test the change itself within the PR. But you can split the workflow change from the rest and marge it to your github fork and then see if it works. Providing a link to a PR on a fork with the workflow change included and working could help with the review.
I think we also have repo made for testing https://github.com/zephyrproject-rtos/zephyr-testing but I never used it. We can discuss how to proceed such changes during testing wg.

@PerMac
Copy link
Member

PerMac commented Nov 14, 2024

To reviewers: this PR requires manual testing before being approved

@PerMac
Copy link
Member

PerMac commented Nov 14, 2024

@hakehuang Please provide more details.

@hakehuang
Copy link
Collaborator Author

is there already differentiation on status transitions for Harness, TestCase, TestSuite, and TestInstance ?

@golowanow , from my observation, the Harness, TestCase, TestSuite and TestInstance transtions can follow one rule set. do you see any exceptions?

@hakehuang
Copy link
Collaborator Author

Please provide more details.

The purposes of this PR:

  1. use statuemachine transition to verify the status change in twister runtime. if there is a violate, TransitionNotAllowed exception will be triggered, and call context will be print.
  2. the transition is defined in the statues.py with class TwisterStatusMachine_inner.cycles, you can generate the status.png by run python3 ./statues.py
  3. with this, we can see and control the status flow to avoid mis-use.
  4. enable call stack print if a exception is triggered, in error.py
  5. fix some status error found by this.

@hakehuang hakehuang force-pushed the order_test_status branch 3 times, most recently from d79db2e to cc42b35 Compare November 14, 2024 17:46
@golowanow
Copy link
Member

golowanow commented Nov 14, 2024

is there already differentiation on status transitions for Harness, TestCase, TestSuite, and TestInstance ?

@golowanow , from my observation, the Harness, TestCase, TestSuite and TestInstance transtions can follow one rule set. do you see any exceptions?

well, potentially they can be in the same space, but according to these comments, there is significant differences in transitions, e.g. only some states/transitions valid for Harness, blocked is TestCase only, etc.

# All statuses below this comment can be used for TestCase
BLOCK = 'blocked'
STARTED = 'started'
# All statuses below this comment can be used for TestSuite
# All statuses below this comment can be used for TestInstance
FILTER = 'filtered'
NOTRUN = 'not run'
# All statuses below this comment can be used for Harness
NONE = None
ERROR = 'error'
FAIL = 'failed'
PASS = 'passed'
SKIP = 'skipped'

@hakehuang
Copy link
Collaborator Author

only some states/transitions valid for Harness, blocked is TestCase only, etc.

I see, let me add harness transition

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO: Exception handling is a good candidate for a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

IMO: Exception handling is a good candidate for a separate PR.

yeah, #80866


class TwisterException(Exception):
pass
def __init__(self, message="TwisterException"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

You pass only a class name in every subclasses, maybe better to add class name to exception message in one place, e.g.:

    def __init__(self, message=None):
        if message is None:
            message = self.__class__.__name__
        else:
            message = f"{self.__class__.__name__}: {message}"
        super().__init__(message)

pass
def __init__(self, message="TwisterException"):
super().__init__(message)
for line in traceback.format_stack():
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can just call logger.exception(msg) to print the callback, however the traceback is usually more relevant when the exception is caught rather than when it is created,

except exceptions.TransitionNotAllowed:
# here we violate the pre-defined transition rules
self.force_state(event)
g()
Copy link
Collaborator

Choose a reason for hiding this comment

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

just use

logger.exception(f"Transition from {self.current_state} to {event} is not allowed.")

instead of adding extra g() method

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, I received
Transition from passed to passed is not allowed
after running:
./scripts/twister -vv --no-clean -T samples/hello_world -p native_sim

using StateMachine in status change.
in stastus.py we can generated the StateMachine
by get_graph

Signed-off-by: Hake Huang <[email protected]>
add pip install step for twister

Signed-off-by: Hake Huang <[email protected]>
add trace print when exception happens

Signed-off-by: Hake Huang <[email protected]>
update test cases for twister
1. test_errors.py add protection.
2. test_handlers.py change call to status
3. test_testsuite.py change call to status

Signed-off-by: Hake Huang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants