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

Fixed cellular unittests #6682

Merged
merged 2 commits into from Apr 23, 2018

Conversation

Projects
None yet
5 participants
@jarvte
Contributor

jarvte commented Apr 19, 2018

Description

Fixed cellular unit tests.
Moved one method from protected to public as it was accidentally set as protected in AT_CellularNetwork. Now that method also can be unit tested.

@AnttiKauppila please review

Internal ref to defect: IOTCELL-799

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested a review from AnttiKauppila Apr 19, 2018

@@ -744,7 +744,8 @@ nsapi_error_t AT_CellularNetwork::get_registration_status(RegistrationType type,
nsapi_error_t AT_CellularNetwork::get_cell_id(int &cell_id)
{
return _cell_id;
cell_id = _cell_id;
return NSAPI_ERROR_OK;

This comment has been minimized.

@AnttiKauppila

AnttiKauppila Apr 20, 2018

Contributor

This is basically an API break, because you are changing the return value

This comment has been minimized.

@0xc0170

0xc0170 Apr 20, 2018

Member

Why method is called get_cell_id but returns only OK status?

Thanks for noticing. @jarvte This should have had PR type Breaking change, cant slip into patch release ⚠️

This comment has been minimized.

@jarvte

jarvte Apr 20, 2018

Contributor

@0xc0170 Actually this fixes the breaking change which was here: a463b07#diff-648c841900b02b662e396ea7f65d382eL651

So from my point of view this should be accepted as this fixes the bug made in earlier merge in above link. It return OK status as we did not wan't to break API when changing getting of cell_id to asynchronous like it should have been.

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Apr 20, 2018

+1 Even though there is a breaking change, the earlier implementation had no meaning, so I don't expect that anyone has actually used it (no issue raised).

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 20, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 20, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 20, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Apr 23, 2018

@cmonr cmonr merged commit 35bd8b9 into ARMmbed:master Apr 23, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-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
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 8809 cycles (-1336 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 23, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 23, 2018

Set this for 5.9.0-rc1 since referenced commit was also tagged for 5.9.0-rc1.

@jarvte jarvte deleted the jarvte:fix_cellular_unittests branch Apr 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment