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

Add Unittest equeue tests #11067

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@Tharazi97
Copy link
Contributor

commented Jul 18, 2019

Description

Ported events queue test to the Greentea standard:
https://github.com/ARMmbed/mbed-os/blob/master/events/equeue/tests/tests.c

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[x] Test update
[ ] Breaking change

Reviewers

@jamesbeyond @maciejbocianski

Release Notes

@Tharazi97 Tharazi97 force-pushed the Tharazi97:equeue_tests branch from d7a3314 to 90c28f9 Jul 18, 2019
@ciarmcom ciarmcom requested review from jamesbeyond, maciejbocianski and ARMmbed/mbed-os-maintainers Jul 18, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Sounds a lot like unit testing.
Should most of the functionality be tested on unit/module testing framework in host machine, instead of spending time on running on embedded devices?

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Sounds a lot like unit testing.
Should most of the functionality be tested on unit/module testing framework in host machine, instead of spending time on running on embedded devices?

It is a valid point that Greentea tests adding CI executing time. How much effort would it be to converting these test to the UNITTEST ? @Tharazi97 @maciejbocianski

Copy link
Member

left a comment

It's a good point, we are trying to push the testing time down so all the not hw specific tests should be offloaded to unittest framework.

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Right I will convert them to gtest.

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

I have converted them to googletests, but they don't work properly on Windows. Does it have to pass all the test on Windows, or is Windows not supported already?

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Not passing on Windows, and passing on Linux, means that you might have just bug that is affected by uninitialised variable.
It is just a difference of compiler and operating system, whether uninitialised are zero, or random values.

Try running your tests using Valgrind to see where the problem is.

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

I have converted them to googletests, but they don't work properly on Windows. Does it have to pass all the test on Windows, or is Windows not supported already?

@Tharazi97
NOT PASS Do you mean the new equeue test? or do you mean the existing UNITTEST ?
all existing UNITTEST should be all passed on Windows, if you git some issue that could mean your environments would have some issues

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Other tests pass. I think that it could be caused by using pthreads library on MinGW, or by wrong reads of ticker function. I'm working on it.

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

I've commited some changes. Unit tests should work on Windows and Linux. But I suggest to leave greentea test as they are, or make them optional to compile, because Equeue is a real time solution and I think it should be able to test it with a ticker and threads available on targets.

@SeppoTakalo @jamesbeyond @maciejbocianski @bulislaw

@@ -1,835 +0,0 @@
/*

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jul 31, 2019

Member

I'm not sure if it's good idea to remove this test file as it's part of equeue library and still runs on CI (https://github.com/ARMmbed/mbed-os/blob/master/.travis.yml#L230). It should be left as is and only disable on travis script.

@@ -1,424 +0,0 @@
/*

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jul 31, 2019

Member

This test file also should stay, as it's part of equeue library and still runs on CI (https://github.com/ARMmbed/mbed-os/blob/master/.travis.yml#L232).

@maciejbocianski

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

@bulislaw @SeppoTakalo @jamesbeyond
I think this is good idea to to leave greentea test as is, and make it optional.
It adds valuable documentation.

What do you think about adding global mechanism/flag to make our test/example codes be aware of running on CI (e.g. MBED_CI_TEST_RUN). That could easily allow to disable specific test/example code when running on CI

Similar flag SKIP_TIME_DRIFT_TESTS was introduced to skip some time drift tests on CI(86dab2f)

#ifndef MBED_CI_TEST_RUN
// when running on CI this test is disabled due to ...

// test code here

#endif 
@jamesbeyond jamesbeyond changed the title added Greentea equeue tests added Unittest equeue tests Aug 1, 2019
@jamesbeyond jamesbeyond requested a review from bulislaw Aug 6, 2019
@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@maciejbocianski @Tharazi97
What is the benefits to keep the GreenTea test?

@OPpuolitaival

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

If we have some benefits of Greentea tests then we can leave those using MBED_EXTENDED_TESTS flag to be make easier to run all tests if wanted. But if there are no benefits of those then those can be removed. Good option is to leave one or two greentea tests as hardware smoke test and remove the rests.

@maciejbocianski

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

What is the benefits to keep the GreenTea test?

The benefit is that we can test whole queue API on target hardware, not only partially as tests-events-queue does

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

What is the benefits to keep the GreenTea test?

The benefit is that we can test whole queue API on target hardware, not only partially as tests-events-queue does

OK, please add the macro as O-P suggested

@Tharazi97 Tharazi97 force-pushed the Tharazi97:equeue_tests branch 3 times, most recently from 5c3dca1 to 60f4d9e Aug 9, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@Tharazi97 Would it make sense to squash (clean the history) - to make a history release ready

@0xc0170 0xc0170 changed the title added Unittest equeue tests Add Unittest equeue tests Aug 9, 2019
@Tharazi97 Tharazi97 force-pushed the Tharazi97:equeue_tests branch from 60f4d9e to 4e2f866 Aug 9, 2019
@Tharazi97 Tharazi97 force-pushed the Tharazi97:equeue_tests branch from 4e2f866 to c06b11b Aug 9, 2019
@0xc0170 0xc0170 added needs: review and removed needs: work labels Aug 9, 2019
@0xc0170
0xc0170 approved these changes Aug 9, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

CI started

@0xc0170 0xc0170 requested a review from maciejbocianski Aug 9, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Aug 10, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

CI fails seems not related.
| LPC55S69_S-ARMC6 | LPC55S69_S | mbed-os-tests-psa-crypto_access_control | TIMEOUT | 1800.7 | default |

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Test was restarted and is all good. I had to restart merge jenkins error, should also be fine (internal CI issue fixed now).

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 12, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Waiting for pr-merge fix, @ARMmbed/mbed-os-test please review

@0xc0170 0xc0170 dismissed bulislaw’s stale review Aug 13, 2019

Converted to unittests

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 13, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

The CI status fixed, this is ready for integration

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@ARMmbed/mbed-os-maintainers Can this be merged?

@0xc0170 0xc0170 merged commit 181f4f7 into ARMmbed:master Aug 20, 2019
19 checks passed
19 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/jenkins/pr-merge Manually fixed status
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/greentea-test Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8648 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@adbridge

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

This is on top of 5.14 changes so moved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.