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

ztest: Add register functionality #37192

Merged
merged 5 commits into from Oct 28, 2021
Merged

Conversation

yperess
Copy link
Collaborator

@yperess yperess commented Jul 26, 2021

Add new functionality to ztest to improve test modularity. The two
primary new entry points are:

  • ztest_register_test_suite
  • ztest_run_registered_test_suites

When registering a new test suite, users provide the name as well as
an optional pragma used to filter the tests for each run. Using NULL
as the pragma ensures that the test is run exactly once (after which
it is removed from the linked list of test suites).

Calls to ztest_run_registered_test_suites take a state pointer as an
argument. This allows the the pragma functions to decide whether the
test should be run (see the BT and accel tests as an example of using
the global test state as a filter).

The biggest benefit of this system (other than the ability to filter
tests and maintain a larger test state) is the ability to better
modularize the test source code. In the example of basicmath, main.c
doesn't need to (or should) know about q7, q15, q31, or f32's test
suites. Instead, each source file manages registering its own test
suite and handling the conditions for running the suite.

Signed-off-by: Yuval Peress peress@chromium.org

@github-actions github-actions bot added area: Test Framework Issues related not to a particular test, but to the framework instead area: Tests Issues related to a particular existing or missing test labels Jul 26, 2021
subsys/testsuite/ztest/src/ztest.c Outdated Show resolved Hide resolved
subsys/testsuite/ztest/src/ztest.c Outdated Show resolved Hide resolved
subsys/testsuite/ztest/src/ztest.c Outdated Show resolved Hide resolved
subsys/testsuite/ztest/src/ztest.c Outdated Show resolved Hide resolved
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

Should add tests under tests/ztest that exercise these new features.

Also, at first glance, there seems to be a fatal flaw in the linked list logic implemented here:
Initial run with current = head, prev = NULL, the first two nodes have NULL pragma

  1. current pragma is NULL and prev is NULL, so head is set to the next node effectively removing it from the next run; prev is now the current (removed) node and current is now the next node.
  2. current pragma is NULL and prev was set to the removed node, so prev's next is set to current's next. But prev has already been removed by updating head to the current node and head still points to the node that should have been removed.

Also the CI didn't run tests/benchmarks/cmsis_dsp/basicmath and is giving a pass -- compilation fails for this test locally.

@yperess
Copy link
Collaborator Author

yperess commented Jul 26, 2021

Done with the requested changes and added unit tests under tests/ztest/register

@yperess yperess requested review from jackrosenthal and removed request for dcpleung September 10, 2021 18:02
@yperess
Copy link
Collaborator Author

yperess commented Sep 28, 2021

Ping on this PR, do we have enough approvals to merge? I understand that the 2.7 release branch has been cut which means we're merging into main now?

@PerMac PerMac self-requested a review September 28, 2021 07:52
@yperess
Copy link
Collaborator Author

yperess commented Sep 29, 2021

@sjg20 @nashif it's still showing that there were requested changes from both of you, if you can, could you PTAL and let me know if anything is missing? This should make it a lot easier to script system level / integration tests.

@yperess
Copy link
Collaborator Author

yperess commented Oct 5, 2021

This PR has had 3 approvals for quite some time now, can it please merge? @nashif can you help with this? I'm really starting to pile on tech debt.

@yperess
Copy link
Collaborator Author

yperess commented Oct 5, 2021

Force push to re-trigger timed out documentation build

@tejlmand
Copy link
Collaborator

tejlmand commented Oct 6, 2021

This PR has had 3 approvals for quite some time now,

I would say it would be nice if some of the code owners of files in this PR would review as well.
@stephanosio @jhedberg @Vudentz @nashif

Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

Minimum CMake is now 3.20.0

tests/ztest/register/CMakeLists.txt Outdated Show resolved Hide resolved
tests/ztest/automain/CMakeLists.txt Outdated Show resolved Hide resolved
@tejlmand
Copy link
Collaborator

tejlmand commented Oct 6, 2021

I notice that @sjg20 has approved, but could you please reply to his comments so that we know why they are not addressed.
For example:
#37192 (comment)
#37192 (comment)
#37192 (comment)

Yuval Peress added 5 commits October 6, 2021 10:36
Add new functionality to ztest to improve test modularity. The two
primary new entry points are:
* ztest_register_test_suite
* ztest_run_registered_test_suites

When registering a new test suite, users provide the name as well as
an optional predicate used to filter the tests for each run. Using NULL
as the predicate ensures that the test is run exactly once (after which
it is automatically filtered from future runs).

Calls to ztest_run_registered_test_suites take a state pointer as an
argument. This allows the the pragma functions to decide whether the
test should be run.

The biggest benefit of this system (other than the ability to filter
tests and maintain a larger test state) is the ability to better
modularize the test source code. Instead of all the various tests
having to coordinate and the main function having to know which tests
to run, each source file manages registering its own test
suite and handling the conditions for running the suite.

Signed-off-by: Yuval Peress <peress@chromium.org>
Introduce a weak implementation of test_main() which calls:
* ztest_run_registered_test_suites(NULL);
* ztest_verify_all_registered_test_suites_ran();

This will attempt to run all registered test suites and verify that
they each ran.

Signed-off-by: Yuval Peress <peress@chromium.org>
Simplify the running and creation of the basicmath benchmark suites.
Using the ztest register functionality, each benchmark registers and
is run from a single call in main.c.

Signed-off-by: Yuval Peress <peress@chromium.org>
Migrating the test suites to the register functionality allows the
main.c source to focus on the state of the system instead of "which"
test to run. In this example, test_main() now calls:
* ut_bt_setup() and updates the state
* common_create_adv_set(), updates the state, and notifies the
  registered tests that the state has been updated which would trigger
  a run.
* common_delete_adv_set(), updates the state, and again notifies
  the registered tests that the state has been updated.

Signed-off-by: Yuval Peress <peress@chromium.org>
Add documentation that details the ability to register test suites and
execute them based on a pragma function. Also the auto-generated
test_main() function.

Signed-off-by: Yuval Peress <peress@chromium.org>
@yperess
Copy link
Collaborator Author

yperess commented Oct 6, 2021

I notice that @sjg20 has approved, but could you please reply to his comments so that we know why they are not addressed. For example: #37192 (comment) #37192 (comment) #37192 (comment)

Missed those, fixed them.

@yperess yperess requested a review from tejlmand October 6, 2021 16:40
@yperess yperess mentioned this pull request Oct 19, 2021
@yperess
Copy link
Collaborator Author

yperess commented Oct 19, 2021

@nashif @tejlmand this PR is getting very stale and I have a lot of additional changes I'd like to make on-top of it to improve testing. Do you have any more feedback on this?

@yperess
Copy link
Collaborator Author

yperess commented Oct 26, 2021

Ping, is anything specific blocking this PR from merging? I've uploaded the next PR on top of this one but it'll look really messy until this one is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Bluetooth area: CMSIS-DSP area: Documentation area: Sensors Sensors area: Test Framework Issues related not to a particular test, but to the framework instead area: Tests Issues related to a particular existing or missing test area: Testsuite Testsuite area: Twister Twister
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants