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.

Sign up for GitHub

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

Jump to bottom

tests: smp: add some module and integration test cases #31010

Merged
merged 1 commit into from
May 6, 2021
Merged

tests: smp: add some module and integration test cases #31010

merged 1 commit into from
May 6, 2021

Conversation

enjiamai
Copy link
Collaborator

@enjiamai enjiamai commented Dec 26, 2020 •
edited
Loading

Add some module and integration test cases for testing SMP:

There are several test cases added in this PR for testing:

Add module test case:

  • test_smp_release_global_lock() and test_smp_release_global_lock_irq()
    to verify z_smp_release_global_lock() works

Add integration test cases:

  • test_inc_concurrency()
    verify parallelly increase operations will fail if not applying synchronization on SMP.

Ps: After clarified by Andy in issue #33551, I remove the test_smp_irq_lock_sem() and test_smp_irq_lock_mutex() because it's better to no use them in this way.

Signed-off-by: Enjia Mai [email protected]

@github-actions github-actions bot added area: Kernel area: Tests Issues related to a particular existing or missing test labels Dec 26, 2020
@LeiW000 LeiW000 requested a review from Zhaoningx January 7, 2021 03:02
@LeiW000
Copy link
Collaborator

LeiW000 commented Jan 7, 2021

@Zhaoningx , please review the PR.

@enjiamai enjiamai force-pushed the coverage-improvement-smp branch 2 times, most recently from 6614aa0 to fb8d00d Compare January 25, 2021 01:08
nashif
nashif requested changes Mar 2, 2021
View reviewed changes
Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

why are we testing those internal APIs directly? Are we trying to hit coverage? Could this be due to dead code? I would prefer to see the functionality and coverage being tested using the public APIs

@enjiamai
Copy link
Collaborator Author

enjiamai commented Mar 2, 2021

why are we testing those internal APIs directly? Are we trying to hit coverage? Could this be due to dead code? I would prefer to see the functionality and coverage being tested using the public APIs

Got it, I understand now. I will correct this and not use this internal API for testing. Thank you!

@enjiamai enjiamai changed the title tests: smp: add test cases for internal API tests: smp: some module and integration test cases for testing SMP Mar 22, 2021
@enjiamai enjiamai requested review from LeiW000 and nashif and removed request for andrewboie March 22, 2021 05:52
@enjiamai enjiamai mentioned this pull request Mar 22, 2021
Closed
@enjiamai enjiamai force-pushed the coverage-improvement-smp branch 6 times, most recently from 36bd57a to 20d044f Compare March 24, 2021 14:20
LeiW000
LeiW000 suggested changes Apr 2, 2021
View reviewed changes
tests/kernel/smp/src/main.c Show resolved Hide resolved
@enjiamai enjiamai force-pushed the coverage-improvement-smp branch 4 times, most recently from ca6f57f to de478b0 Compare April 25, 2021 05:31
@enjiamai
Copy link
Collaborator Author

@nashif @LeiW000 , I have addressed the comments, could you please review this PR again, thanks!

@enjiamai enjiamai requested a review from LeiW000 April 25, 2021 05:59
@enjiamai
Copy link
Collaborator Author

Sorry for did not give it a clear description after the update. I have updated those test cases to better testing scenarios, and also not using the internal API anymore just for coverage. There is no dead code currently(the previous merged PR#33293 has delete the dead code of z_smp_reacquire_global_lock()).

So could you please help to review it again, @nashif @andyross, thank you!

@enjiamai enjiamai changed the title tests: smp: some module and integration test cases for testing SMP tests: smp: add some module and integration test cases Apr 29, 2021
andyross
andyross requested changes May 4, 2021
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Two cases to remove. One that seems needless, the other is validating a behavior that is a historical bug that shouldn't even be valid.

tests/kernel/smp/src/main.c Outdated Show resolved Hide resolved
tests/kernel/smp/src/main.c Show resolved Hide resolved
tests/kernel/smp/src/main.c Outdated Show resolved Hide resolved
tests: smp: add some module and integration test cases
ccb6146 
This PR add 2 module test cases:
- test_smp_release_global_lock() and test_smp_release_global_lock_irq()
  verify z_smp_release_global_lock() works.

And 1 integration test cases:
- test_inc_concurrency() to verify parallelly increase operations will
  fail if not applying synchronization on SMP.

Signed-off-by: Enjia Mai <[email protected]>
@enjiamai enjiamai requested a review from andyross May 5, 2021 15:20
@enjiamai
Copy link
Collaborator Author

enjiamai commented May 5, 2021

Hi, @andyross, Thanks so much for your review. I try to address the comments you gave, could you please take a look at it again?

andyross
andyross approved these changes May 5, 2021
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Can't find anything more to complain about. Thanks!

@enjiamai
Copy link
Collaborator Author

enjiamai commented May 5, 2021

Excuse me, @nashif , could you please take a look at this PR again? I have addressed all the comments and I think it can be merged, thank you!

@nashif nashif merged commit a9edb1f into zephyrproject-rtos:master May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nashif nashif nashif approved these changes

@andyross andyross andyross approved these changes

@LeiW000 LeiW000 LeiW000 approved these changes

@mengxianglinx mengxianglinx Awaiting requested review from mengxianglinx

@Zhaoningx Zhaoningx Awaiting requested review from Zhaoningx

@dcpleung dcpleung Awaiting requested review from dcpleung

Assignees

@andyross andyross

Labels
area: Kernel area: Tests Issues related to a particular existing or missing test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@enjiamai @LeiW000 @nashif @andyross