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

Cellular: More unit tests for ATHandler's read routines #7333

Merged
merged 2 commits into from Jul 11, 2018

Conversation

Projects
None yet
6 participants
@mirelachirica
Contributor

mirelachirica commented Jun 26, 2018

Description

More unit tests for ATHandler's read bytes, string and hexstring routines.

Pull request type

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

@cmonr cmonr requested a review from AriParkkila Jun 27, 2018

consume_char('\"');
if (_last_err) {

This comment has been minimized.

@cmonr

cmonr Jun 27, 2018

Contributor

Why is this check here if it's also being checked 10 lines above?

This comment has been minimized.

@mirelachirica

mirelachirica Jun 27, 2018

Contributor

consume_char() could trigger error if there is nothing to read. In that case if size is 1, this routine would return 0, but it is true that _last_err would be set to error in that case(by consume_char()). I could skip this check and change the unit test to be fine with return value 0 and _last_err set to error.

@cmonr cmonr requested review from kjbracey-arm and AnttiKauppila Jun 27, 2018

@cmonr cmonr added the needs: review label Jun 27, 2018

@AriParkkila

I guess read_string corner cases (like empty string with quotes "\"\"" and null termination when len=size) are tested with Arm internal ref IOTCELL-1095

@@ -100,10 +100,11 @@ class EchoSocket : public UDPSocket {
void test_recvfrom()
{
uint8_t *buf = new uint8_t[_size];
memset(buf, 0, _size);
int recv_size = _size + 1;

This comment has been minimized.

@AriParkkila

AriParkkila Jun 27, 2018

Contributor

Why we need +1 here?

This comment has been minimized.

@mirelachirica

mirelachirica Jun 27, 2018

Contributor

For NULL.

This comment has been minimized.

@mirelachirica

mirelachirica Jun 27, 2018

Contributor

Recvfrom buffer shouldn't have space for NULL. Instead I need to change read_hex_string to not be NULL terminated.

CHECK(-1 == at.read_bytes(buf, 1));
CHECK(NSAPI_ERROR_DEVICE_ERROR == at.get_last_error());
// Return error due to error set to at handler by the above call on empty buffer
CHECK(-1 == at.read_bytes(buf, 1));

This comment has been minimized.

@AriParkkila

AriParkkila Jun 27, 2018

Contributor

This actually returns error even when error is not set above due to buffer is empty.

This comment has been minimized.

@mirelachirica

mirelachirica Jun 27, 2018

Contributor

Right. I was trying to check that read_bytes would return -1 on the _last_err check but it is not possible to track from which case it returns the error.

@@ -461,17 +461,30 @@ ssize_t ATHandler::read(char *buf, size_t size, bool read_even_stop_tag, bool he
}
size_t match_pos = 0;
size_t read_size = hex ? size * 2 : size;
// Size includes NULL
size_t read_size = hex ? (size - 1) * 2 : (size - 1);

This comment has been minimized.

@0xc0170

0xc0170 Jun 27, 2018

Member

how are these changes in ATHandler related to Cellular: More unit tests for ATHandler's read routines ? This is not unit test, thus this is fixing something that unit test reveals? Should be separate commit then

This comment has been minimized.

@mirelachirica

mirelachirica Jun 27, 2018

Contributor

You are correct. I will add new commit + due to comment above:
"I need to change read_hex_string to not be NULL terminated" there will be more changes around this.
I will give up the common read() routine used for read_string and read_hex_string and have own routines with some duplicate code.

This comment has been minimized.

@mirelachirica

mirelachirica Jun 28, 2018

Contributor

I intended to amend the first commit message but had the review changes staged so they made it to the one commit. Is it ok to have this one commit with the updated message?

This comment has been minimized.

@0xc0170

0xc0170 Jun 28, 2018

Member

It would be the best to split them (one is bug fixing, one is testing additions)

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jun 27, 2018

@mirelachirica mirelachirica dismissed stale reviews from AnttiKauppila and AriParkkila via d500749 Jun 28, 2018

@mirelachirica mirelachirica force-pushed the mirelachirica:at_handler_reading_improvements branch 3 times, most recently from 3683833 to a830b4b Jun 28, 2018

Commits updated

@0xc0170 0xc0170 added needs: review and removed needs: work labels Jul 2, 2018

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-wan Jul 2, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 3, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 3, 2018

Build : SUCCESS

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

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.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 5, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@mirelachirica mirelachirica force-pushed the mirelachirica:at_handler_reading_improvements branch from a830b4b to 7de9770 Jul 10, 2018

@mirelachirica

This comment has been minimized.

Contributor

mirelachirica commented Jul 10, 2018

Rebased. Who's review is needed to proceed with this?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 10, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jul 10, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 10, 2018

Build : SUCCESS

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

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.

@cmonr cmonr merged commit c038419 into ARMmbed:master Jul 11, 2018

14 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/astyle Passed, 791 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9046 cycles (-1127 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: CI label Jul 11, 2018

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7333 from mirelachirica/at_handler_reading…
…_improvements

Cellular: More unit tests for ATHandler's read routines
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment