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

Basic test refactoring #5243

Merged
merged 1 commit into from Oct 19, 2017

Conversation

Projects
None yet
6 participants
@maciejbocianski
Member

maciejbocianski commented Oct 3, 2017

Description

No functional changes only refactoring

Status

READY

TESTS/mbedmicro-rtos-mbed/basic/main.cpp Outdated
#include "mbed.h"
#include "greentea-client/test_env.h"
#include "unity.h"

This comment has been minimized.

@pan-

pan- Oct 3, 2017

Member

Please add the module to the inclusion path (see here).

In other words:

#include "unity/unity.h"
#include "utest/utest.h"
#include "rtos/rtos.h"

This comment has been minimized.

@maciejbocianski

maciejbocianski Oct 3, 2017

Member

link is not found

This comment has been minimized.

@pan-

pan- Oct 3, 2017

Member

I'm not sure why you cannot access the guidelines ( @0xc0170 any idea ? ). The guideline state:

* Header files should always be included using the module directory in the path.
* examples: #include “lwip/lwip-interface.h”, #include “drivers/Ticker.h”
* Limit the include path to only the module directory, this allows the module to be moved in the future.
@pan-

Good refactoring. Additional changes might even make it better:

  • Is it necessary to have a second thread executing gt_comm_wait_thread ? That function is the test and can be executed in the test function running in the main thread.
  • A mutex shall protect access to the variable callback_trigger_count. volatile variables cannot be shared safely across different threads of execution.
  • There is two comments explaining what the test do: one at line 19 and the other as the documentation of the test function. It may be good to have a single comment block explaining how things work in detail.
  • Some entities names are not descriptive or misleading: for example callback_trigger_count doesn't mean anything, it is a counter in seconds and it is not invoked from a callback. May you improve entities naming ?
TESTS/mbedmicro-rtos-mbed/basic/main.cpp Outdated
#if defined(MBED_RTOS_SINGLE_THREAD)
#error [NOT_SUPPORTED] test not supported
#endif
using namespace utest::v1;

This comment has been minimized.

@pan-

pan- Oct 3, 2017

Member

Would it be possible to import the symbols needed rather than the whole namespace ?

This comment has been minimized.

@maciejbocianski

maciejbocianski Oct 3, 2017

Member

Generally I don't like importing anything, but rest of TEST code already use it. So I assumed that is prefered

If we don't want importing whole namespace we have to import following symbols:

using utest::v1::status_t;
using utest::v1::Case;
using utest::v1::Specification;
using utest::v1::greentea_test_setup_handler;
using utest::v1::greentea_test_teardown_handler;
using utest::v1::Harness;

This comment has been minimized.

@pan-

pan- Oct 3, 2017

Member

We do not have explicit rule on this item however, importing the minimal set of names used rather than the whole namespace may limit conflicts. We had some in the past with target code.

TESTS/mbedmicro-rtos-mbed/basic/main.cpp Outdated
Thread tick_thread(osPriorityHigh, TEST_STACK_SIZE);
/** Test DUT<->host interaction
Given two threads (A & B) are started and thread A act as counter for time measure

This comment has been minimized.

@pan-

pan- Oct 3, 2017

Member

Could you precise what thread B do ?

TESTS/mbedmicro-rtos-mbed/basic/main.cpp Outdated
/** Test DUT<->host interaction
Given two threads (A & B) are started and thread A act as counter for time measure
When thread B initiates host communication and perform time measure test

This comment has been minimized.

@pan-

pan- Oct 3, 2017

Member

It would be interesting to have a description of the "time measure test".

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 3, 2017

Generally I planned to do minimal changes to align this test to rest of the code from TEST folder.
But if we want to redesign it we can resign from threads and use ticker for time measure

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:basic_tests branch 2 times, most recently Oct 3, 2017

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 3, 2017

@pan- Test was a bit redesigned

@pan-

This comment has been minimized.

Member

pan- commented Oct 3, 2017

It is not only redesigned, from what I see the purpose of the test has also been changed.

Initially the purpose of the test was: Tests is to measure the accuracy of Thread::wait() over a period of time.

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 4, 2017

You are right
After further consideration I realised that after my changes test really test DUT-host communication not basis of RTOS. So I will revert it.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:basic_tests branch Oct 9, 2017

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 9, 2017

@pan- updated after review

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:basic_tests branch to 73a91e0 Oct 9, 2017

@pan-

pan- approved these changes Oct 11, 2017

Thanks for the refactoring

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 11, 2017

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 11, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2017

Build : SUCCESS

Build number : 124
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5243/

Triggering tests

/test mbed-os

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 16, 2017

Build : SUCCESS

Build number : 180
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5243/

Triggering tests

/test mbed-os

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 17, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 17, 2017

Build : SUCCESS

Build number : 197
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5243/

Triggering tests

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 18, 2017

/morph uvisor-test

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 18, 2017

Build : SUCCESS

Build number : 223
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5243/

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

/morph uvisor-test

@0xc0170 0xc0170 merged commit 919e145 into ARMmbed:master Oct 19, 2017

7 checks passed

AWS-CI uVisor Build & Test Success
Details
AWS-CI uvisor Test DIDNT RUN UVISOR TESTS
Details
Cam-CI uvisor Build & Test DIDNT RUN UVISOR TESTS
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment